Skip to content

Conversation

@kroenlein
Copy link
Collaborator

Citrine Python PR

Description

This restores the batching corrections from #747 and updates register_all calls to go to the storables route.

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)
  • Maintenance (non-breaking change to assist developers)

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.
  • I have bumped the version in setup.py

@kroenlein kroenlein requested a review from djack April 21, 2022 04:49
Comment on lines +545 to +552
from citrine.resources.gemd_resource import GEMDResourceCollection
gemd_collection = GEMDResourceCollection(self.project_id, self.dataset_id, self.session)
return gemd_collection.register_all(
models,
dry_run=dry_run,
status_bar=status_bar,
include_nested=include_nested
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All DataConcepts register_all calls should route through GEMDResourceCollection / storables

def register(self, model: DataConcepts, *, dry_run=False) -> DataConcepts:
"""Register a data model object to the appropriate collection."""
return self.gemd.register(model, dry_run=dry_run)
return self.gemd._collection_for(model).register(model, dry_run=dry_run)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All dataset register calls should route through the particular object

def update(self, model: DataConcepts) -> DataConcepts:
"""Update a data model object using the appropriate collection."""
return self.gemd.update(model)
return self.gemd._collection_for(model).update(model)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All DataConcepts update calls should route through the particular collection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Comment on lines +268 to +272
if isinstance(uid, DataConcepts):
collection = self.gemd._collection_for(uid)
else:
collection = self.gemd
return collection.delete(uid, dry_run=dry_run)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dataset.delete will route through the object route if we have a type, and through storables if not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Comment on lines -53 to -76
def update(self, model: DataConcepts) -> DataConcepts:
"""Update a data model object using the appropriate collection."""
return self._collection_for(model).update(model)

def delete(self, uid: Union[UUID, str, LinkByUID, DataConcepts], *, dry_run=False):
"""
Delete a GEMD resource from the appropriate collection.

Parameters
----------
uid: Union[UUID, str, LinkByUID, DataConcepts]
A representation of the resource to delete (Citrine id, LinkByUID, or the object)
dry_run: bool
Whether to actually delete the item or run a dry run of the delete operation.
Dry run is intended to be used for validation. Default: false

"""
model = self.get(uid) # Get full object for collection lookup
return self._collection_for(model).delete(model, dry_run=dry_run)

def register(self, model: DataConcepts, *, dry_run=False) -> DataConcepts:
"""Register a GEMD object to the appropriate collection."""
return self._collection_for(model).register(model, dry_run=dry_run)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are now naturally dispatched through DataConcepts. You get a storables route iff you ask for it.

Comment on lines +353 to +357
# But they would have dispatched differently w/ just a uid
dataset.session.set_response(updated.dump())
dataset.delete(updated.uid)
assert dataset.session.calls[-1].path.split("/")[-3] == basename(GEMDResourceCollection._path_template)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test uid v. object branching. Not actually registered, so repeat delete isn't a problem.

Comment on lines +61 to +62
assert GEMDResourceCollection(collection.project_id, collection.dataset_id, collection.session)._get_path() \
in session.calls[0].path
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though you asked for material_run, it was a register_all so it dispatched to storables.

@kroenlein
Copy link
Collaborator Author

I can verify functionality against stage & dev, but I don't have tests against local-devkit configured.

@kroenlein
Copy link
Collaborator Author

@kroenlein
Copy link
Collaborator Author

Copy link
Contributor

@djack djack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

passes tests, improves the state of the world.

@kroenlein kroenlein merged commit 5d3ba30 into main May 10, 2022
@kroenlein kroenlein deleted the bugfix/no-repeats-in-batch-again branch May 10, 2022 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants