A hypothesis test which loads object template from json file and create hypothesis object #44

ke-zhang-rd · 2019-08-28T19:33:51Z

Hypothesis object created by this process based on container.json will look like

Container(contents={Sample(composition='', description='', name='\U000e4a00\U00064a2a\t\U0008e662\n', projects=[], tags=['\U00056c15\U00086a09𗊚\U000ecaa6\x14\U000f3e75\x00\U001036f9\U000423dd\U000f195a\x06\r\x11\U0001a150\U0003f389\U0010ce9b\x15\x12\U00092be8#\U000f6d39', '\U000fb6fc\x1a\U0010c04e\x14\U000382c8']): 'LOCATION', Sample(composition='', description='', name='\x07#\U000847f8\U000f6578', projects=[], tags=[]): 'LOCATION', Sample(composition='\U00060b73/"', description='\U00046df7\x1d\U000749df(\x1f', name='/ \U000f49a4\U0008d7cf', projects=[], tags=[]): 'LOCATION', Sample(composition='', description='', name='\x17\x1c\x17\x19\x1b', projects=[], tags=['#\x1b\U000be9ef\U0009b5b6', '\x1e\U0007003a', '\U00086d1c\U00107cc6)-.\x1d\U00064fa4', '\U000a42f2\x14\U0007279e', '', '+', '', '\U00107542', '&%""\U000bcb76!\U0001ed63\U000385a0#\U00058815\x06!\U000fd3c0\x01\x1c,𐑄.\x07\U0010f0d4', '\x1a⮊', '', '\U000e7bfd\x16\U0003909e', '\U000cf4bc', '*\x08\x04\x16.\U000f1290\x11\x04ᾈ\x12\U0010d905', '\t\x1f']): 'LOCATION', Sample(composition='', description='', name='\x11\U0003eeaf\U0005c7b7\U0005a192', projects=[], tags=[]): 'LOCATION'}, kind='\U00109223𥉽', name='!! \x15-')

This also could be a beginning point for who want to try using hypothesis test/package later.

danielballan · 2019-08-30T15:13:09Z

amostra/schemas/container.json

    "required": [
        "uuid",
        "revision",
-        "name"
+        "name",
+        "kind",


Why does this testing PR have to change what is required?

In code here, both kind and content needed.
https://github.com/NSLS-II/amostra/blob/master/amostra/objects.py#L175

In design,
contents needed make sense, not sure about kind

it doesn't have really strong connection with this PR. I updated them to make fake container works(has kind and contents).

OK, then let's leave the schema as is. Schema changes should be motivated by user needs, not testing.

I don't understand why you marked this as "resolved".

amostra/schemas/sample.json

amostra/tests/test_jsonschema.py

danielballan · 2019-08-30T15:19:35Z

amostra/tests/test_jsonschema.py

+container_dict = load_schema("container.json")
+container_dict['properties'].pop('uuid')
+container_dict['properties'].pop('revision')
+container_dict['required'] = ['name', 'kind', 'contents']


Same comments as above... it would be good if the only change we had to make here was popping the read-only keys, uuid and revision.

Same reason here. https://github.com/NSLS-II/amostra/pull/44/files#r319613117

I think changing the schema before testing cuts against the spirit of how to use hypothesis. We have to dispense with uuid and required become of how our API works, but it would be better not to mutate anything else about the schema.

amostra/tests/test_jsonschema.py

ke-zhang-rd · 2019-09-03T16:12:07Z

During handling of the above exception, another exception occurred:

KeyError                                  Traceback (most recent call last)
~/miniconda3/envs/py3/lib/python3.7/site-packages/traitlets/traitlets.py in get(self, obj, cls)
    527         try:
--> 528             value = obj._trait_values[self.name]
    529         except KeyError:

KeyError: 'projects'

During handling of the above exception, another exception occurred:

KeyError                                  Traceback (most recent call last)
~/miniconda3/envs/py3/lib/python3.7/site-packages/traitlets/traitlets.py in get(self, obj, cls)
    527         try:
--> 528             value = obj._trait_values[self.name]
    529         except KeyError:

KeyError: 'projects'

During handling of the above exception, another exception occurred:

RecursionError                            Traceback (most recent call last)
~/amostra/amostra/revert_test.py in <module>
      5 db_name = str(uuid.uuid4())
      6 client = amostra.mongo_client.Client('mongodb://localhost:27017/' + db_name)
----> 7 s = client.samples.new(name = '')

~/amostra/amostra/mongo_client.py in new(self, *args, **kwargs)
    147 
    148     def new(self, *args, **kwargs):
--> 149         return self._client._new_document(self._obj_type, args, kwargs)
    150 
    151     def find(self, filter):

~/amostra/amostra/mongo_client.py in _new_document(self, obj_type, args, kwargs)
     59 
     60         # Insert the new object.
---> 61         collection.insert_one(obj.to_dict())
     62 
     63         # Observe any updates to the object and sync them to MongoDB.

~/amostra/amostra/objects.py in to_dict(self)
     57         Represent the object as a JSON-serializable dictionary.
     58         """
---> 59         return {name: getattr(self, name) for name in self.trait_names()}
     60 
     61     @classmethod

~/amostra/amostra/objects.py in <dictcomp>(.0)
     57         Represent the object as a JSON-serializable dictionary.
     58         """
---> 59         return {name: getattr(self, name) for name in self.trait_names()}
     60 
     61     @classmethod

~/miniconda3/envs/py3/lib/python3.7/site-packages/traitlets/traitlets.py in __get__(self, obj, cls)
    554             return self
    555         else:
--> 556             return self.get(obj, cls)
    557 
    558     def set(self, obj, value):

~/miniconda3/envs/py3/lib/python3.7/site-packages/traitlets/traitlets.py in get(self, obj, cls)
    533                 raise TraitError("No default value found for %s trait of %r"
    534                                  % (self.name, obj))
--> 535             value = self._validate(obj, dynamic_default())
    536             obj._trait_values[self.name] = value
    537             return value

~/miniconda3/envs/py3/lib/python3.7/site-packages/traitlets/traitlets.py in _validate(self, obj, value)
    591             value = self.validate(obj, value)
    592         if obj._cross_validation_lock is False:
--> 593             value = self._cross_validate(obj, value)
    594         return value
    595 

~/miniconda3/envs/py3/lib/python3.7/site-packages/traitlets/traitlets.py in _cross_validate(self, obj, value)
    597         if self.name in obj._trait_validators:
    598             proposal = Bunch({'trait': self, 'value': value, 'owner': obj})
--> 599             value = obj._trait_validators[self.name](obj, proposal)
    600         elif hasattr(obj, '_%s_validate' % self.name):
    601             meth_name = '_%s_validate' % self.name

~/miniconda3/envs/py3/lib/python3.7/site-packages/traitlets/traitlets.py in __call__(self, *args, **kwargs)
    905         """Pass `*args` and `**kwargs` to the handler's function if it exists."""
    906         if hasattr(self, 'func'):
--> 907             return self.func(*args, **kwargs)
    908         else:
    909             return self._init_call(*args, **kwargs)

~/amostra/amostra/objects.py in _validate_with_jsonschema(instance, proposal)
     22     This is meant to be used with traitlets' @validate decorator.
     23     """
---> 24     jsonschema.validate(instance.to_dict(), instance.SCHEMA)
     25     return proposal['value']
     26 

... last 8 frames repeated, from the frame below ...

~/amostra/amostra/objects.py in to_dict(self)
     57         Represent the object as a JSON-serializable dictionary.
     58         """
---> 59         return {name: getattr(self, name) for name in self.trait_names()}
     60 
     61     @classmethod

RecursionError: maximum recursion depth exceeded

The error was observed when try to init a Sample with only empty string name

from pymongo import MongoClient
import uuid
import amostra.mongo_client

db_name = str(uuid.uuid4())
client = amostra.mongo_client.Client('mongodb://localhost:27017/' + db_name)
s = client.samples.new(name='')

The reason isn't clear yet. I suspect it come from traitlets's Unicode.
Right now, in sample.json, set "minLength": 1 to go around this error.

danielballan · 2019-09-03T16:31:01Z

Interesting. I think it would be good to understand the cause before merging. Can you start with something minimal like:

class Thing(amostra.objects.AmostraDocument): 
    stuff = traitlets.List(traitlets.Unicode())

ke-zhang-rd · 2019-09-03T17:28:36Z

class Thing(amostra.objects.AmostraDocument):

I'll try. Do you have clue/direction why name(which is Unicode()) empty or not could influence projects which is List(Unicode()) behavior?

ke-zhang-rd · 2019-09-05T18:06:10Z

After some search online, I feel typical way to use __new__ is

def __new__(cls, *args, **kwargs):
    inst = super().__new__(cls)
    '''
    manipulate inst
    '''
    return inst

Also in traitlets examples here, looks validate method bond with instance instead of class.

class Parity(HasTraits):
    value = Int()
    parity = Int()

    @validate('value')
    def _valid_value(self, proposal):

tacaswell · 2019-09-05T21:33:48Z

amostra/objects.py

-        cls._validate = validate(*trait_names)(_validate_with_jsonschema)
-        return super().__new__(cls, *args, **kwargs)
+        instance = super().__new__(cls, *args, **kwargs)
+        instance._validate = validate(*trait_names)(_validate_with_jsonschema)


I think think that this change makes the validation not happen at al! The validate method produces a descriptor which needs to be in place for the super().__new__(...) to find them and do something about it.

Are there any tests we we are sure that we do reject invalid documents?

Yes, I think @tacaswell is right. It would be good to add a test that uses pytest.raises to ensure that validation is running and correctly raising an error on invalid inputs.

I agree with you that validation wasn't triggered.

tacaswell · 2019-09-09T12:45:02Z

Inlt [24]: s                                                                                                                                                                                                                                                    
Out[24]: amostra.objects.Sample

In [25]: s(None, name='')

is enough to trigger the recursion. I suspect this is due to '' being the default value of name and it looks like there is a loop triggered via the interplay interplay between the collective validation during setting the values and during getting the default values.

Given that we have already made 'name' special via the signature, I think we should enforce that it is not the empty sttring in the Sample init.

ke-zhang-rd · 2020-01-31T19:55:18Z

I suspect this need to be fixed?

short words about why recursion
to_dict -> _validate_with_jsonschema -> to_dict ...

ke-zhang-rd · 2020-01-31T20:10:03Z

Maybe I should do thing below instead of every place using to_dict.

    def to_dict(self):
        """
        Represent the object as a JSON-serializable dictionary.
        """
        with self. cross_validation_lock:
            result = {name: getattr(self, name) for name in self.trait_names()}
        return result

danielballan · 2020-01-31T22:05:28Z

Great, using the lock inside to_dict (and removing it from __repr__ and elsewhere) feels right. I will review again with fresh eyes next week. This is too detailed for a Friday 5pm review. :-D

danielballan

I re-opened an old, unresolved comment and left one optional implementation suggestion.

amostra/tests/test_jsonschema.py

danielballan

Looks good. Thanks for your persistence.

ke-zhang-rd added 2 commits August 27, 2019 11:15

ENH: Hypothesis load schema

ae9ac34

MNT: Add some fields to required based on code

9b365d0

ke-zhang-rd requested a review from danielballan August 28, 2019 19:33

ke-zhang-rd force-pushed the jsonschema-test branch from a5da70b to 5b600f2 Compare August 28, 2019 19:51

ke-zhang-rd added 2 commits August 28, 2019 16:04

TST: Add a hypothesis test which loads object template from json

5b600f2

TST: Fix isort error

acf1409

danielballan reviewed Aug 30, 2019

View reviewed changes

ke-zhang-rd added 3 commits August 30, 2019 17:56

MNT: Add projects field

a5e72f9

TST: Update some dict manipulate

83657ae

MNT: Add minLength for name

c4c6733

ke-zhang-rd force-pushed the jsonschema-test branch from 37e9a14 to 0970840 Compare September 3, 2019 15:56

ke-zhang-rd requested a review from danielballan September 3, 2019 15:59

TST: generate more examples and disable too_slow checking

0970840

danielballan reviewed Sep 3, 2019

View reviewed changes

amostra/tests/test_jsonschema.py Outdated Show resolved Hide resolved

amostra/tests/test_jsonschema.py Outdated Show resolved Hide resolved

amostra/tests/test_jsonschema.py Outdated Show resolved Hide resolved

MNT: Override _validate after super construct instance

4a0dea0

tacaswell reviewed Sep 5, 2019

View reviewed changes

MNT: explicit remove uuid and revision

54b84a7

This was referenced Jan 29, 2020

Issue about recursion triggered #51

Closed

Fix max recursion #52

Closed

ke-zhang-rd added 6 commits January 31, 2020 10:48

MNT: Revert to previsou __new__

b3121c5

MNT: Fix empty string bug

4840da5

TES: Try a simple test case

86d3fd0

MNT: pep8 and simplify test

2ef6185

CI: Add pip freeze

3a6623b

MNT: put .to_dict inside cross_validation_lock to break recursion

2ca12e8

CI: Remove pip freeze

f8d6d25

ke-zhang-rd requested review from tacaswell and danielballan January 31, 2020 19:49

MNT: Add with self.cross_validation_lock to to_dict

eb567b4

danielballan reviewed Feb 3, 2020

View reviewed changes

amostra/tests/test_jsonschema.py Outdated Show resolved Hide resolved

MNT: Only name, uuid and revision are required

c3c36b0

ke-zhang-rd force-pushed the jsonschema-test branch from 2fb89fe to 0be3b5b Compare February 3, 2020 19:26

TST: Use itemgetter instead of lambda

0be3b5b

ke-zhang-rd requested a review from danielballan February 3, 2020 19:28

danielballan approved these changes Feb 3, 2020

View reviewed changes

danielballan merged commit 7c6bbb3 into NSLS-II:master Feb 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A hypothesis test which loads object template from json file and create hypothesis object #44

A hypothesis test which loads object template from json file and create hypothesis object #44

ke-zhang-rd commented Aug 28, 2019

danielballan Aug 30, 2019

ke-zhang-rd Aug 30, 2019

ke-zhang-rd Aug 30, 2019

danielballan Aug 30, 2019

danielballan Feb 3, 2020

danielballan Aug 30, 2019

ke-zhang-rd Aug 30, 2019

danielballan Aug 30, 2019

ke-zhang-rd commented Sep 3, 2019

danielballan commented Sep 3, 2019

ke-zhang-rd commented Sep 3, 2019 •

edited

Loading

ke-zhang-rd commented Sep 5, 2019 •

edited

Loading

tacaswell Sep 5, 2019

danielballan Sep 6, 2019

ke-zhang-rd Sep 6, 2019

tacaswell commented Sep 9, 2019

ke-zhang-rd commented Jan 31, 2020

ke-zhang-rd commented Jan 31, 2020

danielballan commented Jan 31, 2020

danielballan left a comment

danielballan left a comment

A hypothesis test which loads object template from json file and create hypothesis object #44

A hypothesis test which loads object template from json file and create hypothesis object #44

Conversation

ke-zhang-rd commented Aug 28, 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

ke-zhang-rd commented Sep 3, 2019

danielballan commented Sep 3, 2019

ke-zhang-rd commented Sep 3, 2019 • edited Loading

ke-zhang-rd commented Sep 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Sep 9, 2019

ke-zhang-rd commented Jan 31, 2020

ke-zhang-rd commented Jan 31, 2020

danielballan commented Jan 31, 2020

danielballan left a comment

Choose a reason for hiding this comment

danielballan left a comment

Choose a reason for hiding this comment

ke-zhang-rd commented Sep 3, 2019 •

edited

Loading

ke-zhang-rd commented Sep 5, 2019 •

edited

Loading