Fixes to schema v2 #255

hardbyte · 2019-06-07T07:50:05Z

Schema v2 could have been better

codecov · 2019-06-09T00:54:11Z

Codecov Report

Merging #255 into release-v0.13.0 will increase coverage by 6.33%.
The diff coverage is 73.97%.

@@                 Coverage Diff                 @@
##           release-v0.13.0     #255      +/-   ##
===================================================
+ Coverage            86.99%   93.32%   +6.33%     
===================================================
  Files                   17       17              
  Lines                 1138     1199      +61     
===================================================
+ Hits                   990     1119     +129     
+ Misses                 148       80      -68

hardbyte · 2019-06-09T03:48:26Z

Note the tests are failing because of the tutorials - that will be addressed in #257

clkhash/bloomfilter.py

hardbyte · 2019-06-11T01:10:13Z

clkhash/cli.py

+        log("schema is valid", color='green')
+
+    except SchemaError as e:
+        log(str(e))


This prints out a pretty detailed message about what is invalid.

$ python -m clkhash validate-schema tests\testdata\bad-schema-v2.json The schema is not valid. {'identifier': 'DOB YYYY/MM/DD', 'format': {'type': 'date', 'description': 'Numbers separated by slashes, in the year, month, day order', 'format': '%Y/%m/%d'}, 'hashing': {'ngram': 1, 'positional': True, 'k': 30, 'hash': {'type': 'doubleHash'}}} is not valid under any of the given schemas Failed validating 'oneOf' in schema['properties']['features']['items']: {'oneOf': [{'$ref': '#/definitions/featureConfig'}, {'$ref': '#/definitions/ignoreFeature'}], 'type': 'object'} On instance['features'][2]: {'format': {'description': 'Numbers separated by slashes, in the year, ' 'month, day order', 'format': '%Y/%m/%d', 'type': 'date'}, 'hashing': {'hash': {'type': 'doubleHash'}, 'k': 30, 'ngram': 1, 'positional': True}, 'identifier': 'DOB YYYY/MM/DD'}

will it show all the errors or just the first one?

jsonschema just outputs the first error - although with a small tweak it could iterate over them. https://python-jsonschema.readthedocs.io/en/stable/validate/#jsonschema.IValidator.iter_errors

hardbyte · 2019-06-11T01:41:21Z

clkhash/cli.py

+    try:
+        schema_object = clkhash.schema.from_json_file(schema_file=schema)
+    except SchemaError as e:
+        log(str(e))


As with the new validate-schema command this now prints out quite a detailed error message based on the schema violation.

clkhash/field_formats.py

hardbyte · 2019-06-11T01:45:03Z

clkhash/field_formats.py

-    if not num_bits and not k:
-        num_bits = 200 # default for v2 schema
+
+    num_bits = hashing_strategy.get('numBits')


Note if the user didn't provide a hashing strategy the default includes numBits=200 (from the master schema)

hardbyte · 2019-06-11T01:49:09Z

clkhash/field_formats.py

+        type_str = json_dict['format']['type']
+    except KeyError as e:
+        error = InvalidSchemaError("Missing attribute " + str(e))
+        error.json_field_spec = json_dict


By storing the raw data that was invalid we can provide much better error messages to the user.

hardbyte · 2019-06-11T01:52:11Z

clkhash/schema.py

@@ -113,7 +129,8 @@ def convert_feature(f):
        if 'weight' in hashing:
            del hashing['weight']

-        hashing['k'] = int(round(weight * k))
+        hashing['strategy'] = {}


This strategy key was missing from the converted schemas. The k was ignored because extra attributes were allowed.

hardbyte · 2019-06-11T01:56:54Z

clkhash/schema.py

+    feature_errors = []
+    feature_configs = []
+
+    for i, feature_config in enumerate(dct['features']):


So we can report which i feature/s have problems.

hardbyte · 2019-06-11T02:01:31Z

clkhash/schema.py

-        msg = ('The master schema is not a valid JSON file. The schema cannot '
-               'be validated. Please file a bug report.')
-        raise_from(MasterSchemaError(msg), e)
+    master_schema = _get_master_schema(version)


Moved this bytes -> json handling into the helper function.

hardbyte · 2019-06-11T02:03:01Z

clkhash/schemas/v2.json

@@ -136,6 +141,7 @@
    },
    "hashingConfig": {
      "type": "object",
+      "additionalProperties": false,


This would have caught the invalid schemas.

or we could modify line 145 to "required": ["ngram", "strategy"],
as it doesn't really make sense to have a default here anyway.

hardbyte · 2019-06-11T07:44:57Z

~~I have a local branch which updates the docs further~~ rebased and pushed that too.

Note the current published docs refers to v1

wilko77

more work to be done

clkhash/schemas/v2.json

wilko77 · 2019-06-11T08:05:25Z

clkhash/cli.py

+        log("schema is valid", color='green')
+
+    except SchemaError as e:
+        log(str(e))


will it show all the errors or just the first one?

clkhash/field_formats.py

wilko77 · 2019-06-12T04:11:02Z

clkhash/field_formats.py

        :returns: An initialised instance of the appropriate FieldSpec
            subclass.
    """
-    if 'ignored' in json_dict:
+    if 'ignored' in json_dict and json_dict['ignored']:


that cannot happen if you validate against schema. We enforce True with the const thing.

wilko77 · 2019-06-12T04:16:00Z

clkhash/schemas/v2.json

@@ -136,6 +141,7 @@
    },
    "hashingConfig": {
      "type": "object",
+      "additionalProperties": false,


or we could modify line 145 to "required": ["ngram", "strategy"],
as it doesn't really make sense to have a default here anyway.

docs/schema.rst

hardbyte · 2019-06-13T12:50:07Z

Thanks for the review Wilko. I've created a bunch of issues and tried not to adjust the schema itself in this PR. I wanted to mostly tighten the spec up and ensure all our examples/tests were compliant.

Shame I didn't pick up on the strategy issue back here

Only one thing you said I had an issue with:

that cannot happen if you validate against schema. We enforce True with the const thing.

I think that ignored=False should be valid for any feature so someone can easily toggle a feature on and off.

wilko77 · 2019-06-17T01:20:16Z

I see. Good point.
So an ignoreFeature section needs to have ignored: true, but any other featureConfig can have an optional ignored: true/false.
We should probably make this explicit in the featureConfig definition and also test for the correct behavior.

wilko77 · 2019-06-17T01:32:26Z

as you very elegantly diverted all my concerns into separate issues, you could sent the PR again for me to sign off.

clkhash/schema.py

clkhash/schemas/v2.json

docs/schema.rst

hardbyte · 2019-06-25T13:07:16Z

requirements.txt

@@ -5,7 +5,7 @@ cryptography==2.6.1
 enum34==1.1.6; python_version < '3.4'
 future==0.17.1
 futures==3.2.0; python_version < '3.2'
-jsonschema==2.6.0
+jsonschema==3.0.1


Something, quite possibly this, has broken the pyinstaller - https://ci.appveyor.com/project/hardbyte/clkhash/history

Reverting back to jsonschema 2.6 seems to have fixed the windows exe build - https://ci.appveyor.com/project/hardbyte/clkhash/builds/25527663

…ith the spec

Fixes #254 Another tweak to schema v2 Add a short note about the v2 schema in the docs.

Add cli command to validate a schema Avoid a divide by zero when ngrams is empty cli can throw nice errors when asked to hash with invalid schema schema types

Previously duplicated default information in schema and code.

Update to latest jsonschema version

* Save run of CLI tutorial against newest anonlink server * Include a notebook sanitizer so travis can test cells match. * Use the new version of anonlink in api tutorial. * fine-tuned schemas for jaw dropping performance demonstration * Schema id now includes version * Update author attribute

* Make the v2 schema stricter with additional properties * Fix the v2 testdata schemas to be compliant with the specification * Fix the hardcoded feature configs in the unit tests to be compliant with the spec * Bugfix in converting schemas from v1 to v2 * Update documentation regarding linkage schema v2 * Add a v2 schema with errors * Invalid schema exceptions keep more context * Add cli command to validate a schema and cli schema validation tests * Avoid a divide by zero when ngrams is empty * cli can throw nice errors when asked to hash with invalid schema * Travis: notebook execution runs in the integration test stage * Expose Schema at top level * Don't include a default hashing strategy in the schema * Update tutorial notebooks to use v2 schema (#257) * Include a notebook sanitizer so travis can test cells match. * Use the new version of anonlink in api tutorial. * fine-tuned schemas for jaw dropping performance demonstration * Schema id now includes version * Update author attribute * Note we don't update to latest jsonschema version as Windows exe built with PyInstaller failed. Closes #254

hardbyte requested a review from wilko77 June 7, 2019 07:50

hardbyte force-pushed the fix-schema-v2 branch from 81ec2ea to 0e87136 Compare June 9, 2019 00:54

hardbyte force-pushed the fix-schema-v2 branch 2 times, most recently from 0259e01 to 1fba6b7 Compare June 11, 2019 01:07

hardbyte commented Jun 11, 2019

View reviewed changes

wilko77 requested changes Jun 12, 2019

View reviewed changes

This was referenced Jun 13, 2019

Duplicated defaults #258

Open

Rename strategy properties in schema v2 #259

Closed

hardbyte requested a review from wilko77 June 22, 2019 22:26

wilko77 approved these changes Jun 24, 2019

View reviewed changes

clkhash/schema.py Outdated Show resolved Hide resolved

clkhash/schemas/v2.json Outdated Show resolved Hide resolved

docs/schema.rst Outdated Show resolved Hide resolved

hardbyte commented Jun 25, 2019

View reviewed changes

hardbyte self-assigned this Jun 26, 2019

hardbyte force-pushed the release-v0.13.0 branch from f49f0e1 to 29d8da7 Compare June 26, 2019 01:26

hardbyte added 11 commits June 26, 2019 11:26

Tweak schema v2 to catch hash config errors additional properties

a65c4f4

Fix the v2 testdata schemas to be compliant with the specification

2b75199

Fix the hardcoded feature configs in the unit tests to be compliant w…

0340e80

…ith the spec

Only ignore if the ignored boolean is true...

69a8a6c

Bugfix in converting schemas from v1 to v2

555a21b

Fixes #254 Another tweak to schema v2 Add a short note about the v2 schema in the docs.

Add a v2 schema with errors

a298d18

Invalid schema exceptions keep more context

806a3cd

Schema updates

a421c82

Add cli command to validate a schema Avoid a divide by zero when ngrams is empty cli can throw nice errors when asked to hash with invalid schema schema types

Expose Schema at top level

b61362d

Travis: notebook execution runs in the integration test stage

5338104

Add cli schema validation tests

d524a37

hardbyte and others added 12 commits June 26, 2019 11:28

_get_master_schema now returns a json objects instead of bytes

58047b0

Load default hashing strategy from master schema

2b2748f

Previously duplicated default information in schema and code.

Minor docstring update

8fbc6b8

Update another not quite compliant example schema

9b90959

Make the v2 schema stricter with additional properties

bcf8fc3

Update to latest jsonschema version

Linkage schema documentation update

0f897e0

Don't include a default hashing strategy in the schema

54f921b

Update to latest jsonschema version

69bf6a1

Update docstring and require non optional strategy key

349770d

Minor changes to schema v2

cd5ce90

Adjusted test and v1 to v2 conversion to comply with stricter schema

57fc7fe

hardbyte force-pushed the fix-schema-v2 branch from f3a933f to b4a4a1a Compare June 26, 2019 01:29

hardbyte added 2 commits June 26, 2019 11:56

Fix merge/rebase error

b0575bc

Store field spec on exception

61bf256

hardbyte merged commit 2f9f9f1 into release-v0.13.0 Jun 26, 2019

hardbyte deleted the fix-schema-v2 branch June 26, 2019 04:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes to schema v2 #255

Fixes to schema v2 #255

hardbyte commented Jun 7, 2019 •

edited

Loading

codecov bot commented Jun 9, 2019 •

edited

Loading

hardbyte commented Jun 9, 2019

hardbyte Jun 11, 2019

wilko77 Jun 11, 2019

hardbyte Jun 13, 2019

hardbyte Jun 11, 2019

hardbyte Jun 11, 2019

hardbyte Jun 11, 2019

hardbyte Jun 11, 2019

hardbyte Jun 11, 2019

hardbyte Jun 11, 2019

hardbyte Jun 11, 2019

wilko77 Jun 12, 2019

hardbyte commented Jun 11, 2019 •

edited

Loading

wilko77 left a comment

wilko77 Jun 11, 2019

wilko77 Jun 12, 2019

wilko77 Jun 12, 2019

hardbyte commented Jun 13, 2019 •

edited

Loading

wilko77 commented Jun 17, 2019

wilko77 commented Jun 17, 2019

hardbyte Jun 25, 2019

hardbyte Jun 26, 2019

Fixes to schema v2 #255

Fixes to schema v2 #255

Conversation

hardbyte commented Jun 7, 2019 • edited Loading

codecov bot commented Jun 9, 2019 • edited Loading

Codecov Report

hardbyte commented Jun 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hardbyte commented Jun 11, 2019 • edited Loading

wilko77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hardbyte commented Jun 13, 2019 • edited Loading

wilko77 commented Jun 17, 2019

wilko77 commented Jun 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hardbyte commented Jun 7, 2019 •

edited

Loading

codecov bot commented Jun 9, 2019 •

edited

Loading

hardbyte commented Jun 11, 2019 •

edited

Loading

hardbyte commented Jun 13, 2019 •

edited

Loading