Major refactor of the taurus <-> citrine-python interaction by maxhutch · Pull Request #223 · CitrineInformatics/citrine-python

maxhutch · 2020-02-18T00:07:31Z

Citrine Python PR

Description

This PR is a major refactor of the way that citrine-python uses taurus. In taurus 0.6.0, the json serializer/deserializer was encapsulated in the TaurusJson() class. This allowed for the register_classes method to be exposed, which allows the class dictionary of a TaurusJson object to be updated. This change uses register_classes to override the deserialization of taurus objects like MaterialRun and ProcessTemplate to their citrine-python resource counterparts. That way, the taurus loads/dumps methods can be used to reconstruct material histories from the response of the material history route, which significantly simplifies the logic contained here in this repo (-923 lines!).

This leads to two other semi-significant changes:

The citrine.attributes no longer need to be duplicated from taurus; the taurus versions will do fine on their own
Storable was therefore rolled back into DataConcepts and the logic to include audit_info was moved there.

There are some consequences to the json format due to the upgrade from taurus 0.4.1 to 0.6.0, which are communicated in v0.5.0 release notes and v0.6.0 release notes.

PR Type:

Breaking change (fix or feature that would cause existing functionality to change)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Adherence to team decisions

I have added tests for 100% coverage
I have written Numpy-style docstrings for every method and class.
I have communicated the downstream consequences of the PR to others.

See the PR CitrineInformatics/gemd-python#86

maxhutch · 2020-02-19T03:07:55Z


+    encoder = None
+
    def __init__(self, typ: str):


audit_info was moved here from Storable

maxhutch · 2020-02-19T03:08:17Z

+    @classmethod
+    def from_dict(cls, d: dict):
+        """Build a data concepts option from a dictionary."""
+        audit_info = d.pop("audit_info", None)


This is a general pattern for if we want to add fields to taurus objects

What do you think about having something like

citrine_python_fields = {'audit_info': AuditInfo}

at the class level and looping over that in this method?

maxhutch · 2020-02-19T03:08:42Z

-            setattr(dc_linked_obj, reverse_field, dc_obj)
-            # Re-establish the link between linked_obj and obj_with_soft_links
-            setattr(linked_obj, reverse_field, obj_with_soft_links)
+        return cls.get_encoder().copy(data)


maxhutch · 2020-02-19T03:09:24Z

+            DataConcepts._make_class_dict()
+            cls.encoder = TaurusJson()
+            cls.encoder.register_classes(
+                {k: v for k, v in DataConcepts.class_dict.items() if k != "link_by_uid"}


This is where we override the taurus objects with their citrine-python equivalents

maxhutch · 2020-02-19T03:10:24Z

-        return MaterialRun.build(model)
+
+        # Add the root to the context and sort by writable order
+        blob = dict()


This reconstructs the json serialization format so that json deserialization can be used to build the connected material history

maxhutch · 2020-02-19T03:10:54Z

+from taurus.util import flatten
+
+
+def test_flatten():


This is the originating test that we wanted to get passing.

maxhutch · 2020-02-19T03:21:17Z

~~Note that this will require https://github.com/CitrineInformatics/nextgen-devkit/pull/302~~

kroenlein

Generally looks good.

kroenlein · 2020-02-18T00:32:01Z

@@ -1,4 +1,4 @@
-taurus-citrine==0.4.1
+git+https://github.com/CitrineInformatics/taurus.git@develop


Why is the official requirements file pulling in develop?

That's outdated; it was just for testing before the taurus 0.6.0 release.

maxhutch · 2020-02-19T05:46:20Z

I'm wondering if we shouldn't leave in citrine.attributes as deprecated aliases.

bfolie · 2020-02-19T06:06:33Z

+            :py:mod:`JSON encoder <taurus.jsonr>`. The ensuing dictionary must
            have a `type` field that corresponds to the response key of this class or of
            :py:class:`LinkByUID <taurus.entity.link_by_uid.LinkByUID>`.
        session: Session


@bfolie

h/t @bfolie and @kroenlein

kroenlein

Generally agree. There's a residual

from taurus.client.json_encoder import loads, dumps

in tests/serialization/test_attribute_template.py that does not appear to be intentional. Should it be scrubbed?

asantas93

Huge fan of this. Left a few small comments.

asantas93 · 2020-02-19T14:45:47Z

-            The name of the field
+        audit_info = d.pop("audit_info", None)
+        obj = super().from_dict(d)
+        obj.skip.add("_audit_info")


I'm a little put off by mutations to skip -- isn't it a class variable?

Looks like we just don't need this.

asantas93 · 2020-02-19T14:49:19Z

+    @classmethod
+    def from_dict(cls, d: dict):
+        """Build a data concepts option from a dictionary."""
+        audit_info = d.pop("audit_info", None)


What do you think about having something like

citrine_python_fields = {'audit_info': AuditInfo}

at the class level and looping over that in this method?

asantas93 · 2020-02-19T14:54:43Z

+            data['context'] + [data['root']],
+            key=lambda x: writable_sort_order(x["type"])
+        )
+        root_uid = next(iter(data['root']['uids'].items()))


Suggested change

root_uid = next(iter(data['root']['uids'].items()))

root_scope, root_id = next(iter(data['root']['uids'].items()))

maybe?

asantas93 · 2020-02-19T14:56:32Z

+        blob["context"] = sorted(
+            data['context'] + [data['root']],
+            key=lambda x: writable_sort_order(x["type"])
+        )


asantas93 · 2020-02-19T15:01:53Z

+
+        # Serialize using normal json (with the TaurusEncoder) and then deserialize with the
+        # TaurusJson encoder in order to rebuild the material history
+        return MaterialRun.get_encoder().loads(json.dumps(blob, cls=TaurusEncoder, sort_keys=True))


Is there a way to move from python primitive types to citrine-python types directly? In my dream world git grep 'loads.*dumps' exits with failure.

I think we'd want to dive into get_resource to deserialize using get_json_support the first time rather than going round-robin through the string again. Let's leave that out of scope for this PR, though? It'd be an easy non-breaking change to make in the future.

asantas93 · 2020-02-19T16:00:04Z

+            if popped[field] is None:
+                setattr(obj, "_{}".format(field), None)
+            elif isinstance(popped[field], dict):
+                setattr(obj, "_{}".format(field), clazz.build(popped[field]))


I think we probably want to fall back on clazz(popped[field]) when hasattr(clazz, 'build') is false. That would allow us to use primitive types.

Actually there's probably a neater way than hasattr. Either way I think we can probably use that and ditch the is instance of dict check above.

issubclass seems to be the thing I'm thinking of instead of hasattr

Can we cross that bridge when we get there? We'll have more information when we next decide to add a client_specific_field.

bfolie

❤️

maxhutch added 3 commits February 17, 2020 15:53

Stashing a working codeset

a249eb3

Another sync

e343b02

Remove attributes, deferring to taurus

a9fcef5

maxhutch mentioned this pull request Feb 18, 2020

Extending writable sort order to be public CitrineInformatics/gemd-python#85

Merged

maxhutch added 3 commits February 17, 2020 19:38

Remove Storable

a468782

Add flatten test

fdaa809

Updating in sync with the encapsulated taurus json

e571213

See the PR CitrineInformatics/gemd-python#86

maxhutch mentioned this pull request Feb 19, 2020

Include the output material when serializing processes CitrineInformatics/gemd-python#87

Merged

maxhutch added 4 commits February 18, 2020 21:20

Use taurus-citrine==0.6.0

e6109f4

Get coverage back to 100%

4b73f8b

Appease flake8

8d29c6c

Make sure context is serialized before object

306d2c7

maxhutch marked this pull request as ready for review February 19, 2020 03:07

maxhutch commented Feb 19, 2020

View reviewed changes

Remove debugging prints

59b50d7

maxhutch changed the title ~~Major refactor of the taurus <-> citrine-python interaction (WIP)~~ Major refactor of the taurus <-> citrine-python interaction Feb 19, 2020

maxhutch requested review from bfolie, kroenlein and spaugh February 19, 2020 03:12

maxhutch added the do not merge label Feb 19, 2020

kroenlein previously approved these changes Feb 19, 2020

View reviewed changes

Add back citrine.attributes as deprecated aliases.

610bce3

maxhutch dismissed kroenlein’s stale review via 610bce3 February 19, 2020 05:57

bfolie reviewed Feb 19, 2020

View reviewed changes

maxhutch added 2 commits February 19, 2020 01:44

Improving doc coverage to respond to comments

0c505b0

h/t @bfolie and @kroenlein

Appease flake8

4bab768

maxhutch removed the do not merge label Feb 19, 2020

maxhutch requested review from bfolie and kroenlein February 19, 2020 06:57

kroenlein previously approved these changes Feb 19, 2020

View reviewed changes

asantas93 previously approved these changes Feb 19, 2020

View reviewed changes

Pull audit info into client_specific_fields

5cd77ff

maxhutch dismissed stale reviews from asantas93 and kroenlein via 5cd77ff February 19, 2020 15:53

maxhutch added 2 commits February 19, 2020 10:56

Use better intermediate variable names in get_history

e95b199

Remove latent reference to taurus.client.json_encoder

dcb736c

maxhutch requested review from asantas93 and kroenlein February 19, 2020 15:57

asantas93 reviewed Feb 19, 2020

View reviewed changes

asantas93 approved these changes Feb 19, 2020

View reviewed changes

bfolie approved these changes Feb 19, 2020

View reviewed changes

maxhutch merged commit 5d64872 into develop Feb 19, 2020

maxhutch deleted the bugfix/flatten-fails-on-resource-redux branch February 19, 2020 17:45

maxhutch mentioned this pull request Feb 19, 2020

Citrine v0.8.0 is released! #222

Merged

		@@ -1,4 +1,4 @@
		taurus-citrine==0.4.1
		git+https://github.com/CitrineInformatics/taurus.git@develop

	root_uid = next(iter(data['root']['uids'].items()))
	root_scope, root_id = next(iter(data['root']['uids'].items()))

Conversation

maxhutch commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Citrine Python PR

Description

PR Type:

Adherence to team decisions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxhutch commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kroenlein left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maxhutch commented Feb 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kroenlein left a comment

Choose a reason for hiding this comment

Uh oh!

asantas93 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bfolie left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maxhutch commented Feb 18, 2020 •

edited

Loading

maxhutch commented Feb 19, 2020 •

edited

Loading