Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
May 28, 2024

Conversation

BryanFauble
Copy link
Contributor

@BryanFauble BryanFauble commented May 15, 2024

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: [SYNPY-1356] Updating logic around activity/annotations to be optional #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 BryanFauble changed the title [SYNPY-1356] Integration test case for removing activity via manifest file [SYNPY-1356] Logic around disassociating Activity from File Entity May 16, 2024
@BryanFauble
Copy link
Contributor Author

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?

@BryanFauble BryanFauble marked this pull request as ready for review May 16, 2024 00:45
@BryanFauble BryanFauble requested a review from a team as a code owner May 16, 2024 00:45
Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

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

Thanks Bryan - the code looks great, but just wanted to have some discussions around some of the comments I made.

"parent": folder.id,
"used": "",
"executed": "",
"activityName": "",
Copy link
Member

Choose a reason for hiding this comment

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

I know this is existing behavior but what would happen if an annotation was "activityName" ?

@BryanFauble BryanFauble requested a review from a team May 23, 2024 18:03
Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

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

🔥 LGTM! Going to pre-approve, there's some comments but they are minor.

synapseclient/core/utils.py Show resolved Hide resolved
@pep8speaks
Copy link

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)

@BryanFauble BryanFauble merged commit 4dd2dfc into develop May 28, 2024
15 checks passed
@BryanFauble BryanFauble deleted the SYNPY-1356-activity-deletion-in-manifest branch May 28, 2024 18:28
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.

None yet

3 participants