Sage-Bionetworks / synapsePythonClient

Programmatic interface to Synapse services for Python
https://www.synapse.org
Apache License 2.0
65 stars 67 forks source link

[SYNPY-1356] Logic around disassociating Activity from File Entity #1098

Closed BryanFauble closed 1 month ago

BryanFauble commented 1 month ago

Problem:

  1. When you remove an activity from a file within a manifest (Or through the OOP model interface) it does not delete the Activity off the entity. It will only remove the activity relationship to new versions of that entity, since activity relationships are not brought forward onto new entity versions automatically. The old syncToSynapse relied on this behavior as every sync (Even if a file within the manifest did not change) caused a version update to occur.

Solution:

  1. Note: A large portion of these changes were already reviewed via this PR: https://github.com/Sage-Bionetworks/synapsePythonClient/pull/1099
  2. Added in some (unfortunately hacky) backwards compatible code that will handle disassociating an Activity from an entity when the used/executed fields are present in a manifest upload, but no values for those cells.
  3. Updated the logic around when a version of a File entity is updated to match the OOP model. The OOP model works by only causing a version update on an Entity if there is metadata about the Entity that changed.

Testing:

  1. Integration testing around this logic
  2. I also verified that I am able to go onto the master branch (Before the syncToSynapse changes) and run these integration tests (With a minor update to storable_container to retrieve the activity for the loaded file). This means that behavior wise according these integration tests before/after is working the same*, with 1 small cavet around entity versioning detailed above in the Solution.
BryanFauble commented 1 month ago

Hey @brucehoff I could use your feedback on these changes. Some TLDR on the history of these changes:

During updates of data for new Object Models I have this conditional logic in place and am expanding this to the utility syncToSynapse function:

  1. If no update to an Entity occurred where we need to make a call to: PUT /entity/{id} I am not automatically requesting the version number of the entity is incremented.
  2. Updates to the version number by Synapse back-end is ok, no issues there

Historically in the client:

  1. If syn.store() was called on an Entity - the Python client would always update the version number for that entity even if no data on that entity actually changed.

Some themed questions:

  1. Does it make sense to have this logic in place in the client to limit when a version update occurs to only when data has changed?
  2. Should a manifest upload increment a file entity even if there was no update to that entity?
pep8speaks commented 1 month ago

Hello @BryanFauble! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 444:89: E501 line too long (105 > 88 characters) Line 764:89: E501 line too long (91 > 88 characters)