[SYNPY-1398] Support .[version] syntax for input SynIDs

[jaymedina]

problem

syncToSynapse fails for provenance pointing to a specific version of a synID using syn[id].[version] notation (e.g. syn1234.5). This revisits the discussion on the Python client adding support for synID.[version] notation.

---

solution

The scope of this PR is constrained to allowing .version syntax in syncTo/FromSynapse syn.get and synapse get for the CLI

• syncToSynapse supports .[version] notation
• syncFromSynapse supports .[version] notation
• syn.get supports .[version] notation
• synapse get command supports .[version] notation
• Corresponding documentation is updated to show new support
• New unit/integration tests for feature coverage and error handling

---

testing & preview

support for syn.get(...) :

support for synapse get for CLI :

(synapsePythonClient) (synapseclient) bash-3.2$ synapse get syn53182713
Downloaded file: foo.txt
Creating /Users/jmedina/Documents/forks/synapsePythonClient/jout/foo.txt
(synapsePythonClient) (synapseclient) bash-3.2$ mv foo.txt foo_v2.txt
(synapsePythonClient) (synapseclient) bash-3.2$ synapse get syn53182713.1
Downloaded file: foo.txt
Creating /Users/jmedina/Documents/forks/synapsePythonClient/jout/foo.txt
(synapsePythonClient) (synapseclient) bash-3.2$ cat foo.txt
VERSION 1
(synapsePythonClient) (synapseclient) bash-3.2$ cat foo_v2.txt 
VERSION 2
(synapsePythonClient) (synapseclient) bash-3.2$ 

support for syncFromSynapse :

support for syncToSynapse :

[pep8speaks]

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

• In the file synapseclient/client.py:

Line 1027:89: E501 line too long (120 > 88 characters)
Line 1028:89: E501 line too long (117 > 88 characters)
Line 1031:89: E501 line too long (104 > 88 characters)

• In the file tests/unit/synapseclient/core/unit_test_utils.py:

Line 112:20: E711 comparison to None should be 'if cond is None:'

Comment last updated at 2024-01-25 22:27:28 UTC

[jaymedina]

I had a to-do item in the description for deprecating the version arg in synapse.get but I think it's best to leave that out until the full codebase supports .version syntax, what do you think @BryanFauble

[thomasyu888]

I had a to-do item in the description for deprecating the version arg in synapse.get but I think it's best to leave that out until the full codebase supports .version syntax, what do you think @BryanFauble

I think that is a good idea, but I think that would be a pretty big breaking change, and would require stakeholder feedback. Let's hold off for now until I can gather some of that feedback.

One thought I had was to suggest the API itself to support the dot notation. I think both the web and this current feature branch are extracting the version and feeding it to the API.

Another thing is this:

entity = syn.get("syn1234.3")
# entity.id = "syn1234"

The get API endpoint returns this object: https://rest-docs.synapse.org/rest/org/sagebionetworks/repo/model/FileEntity.html.

[jaymedina]

One thought I had was to suggest the API itself to support the dot notation.

I think it's ideal for the Python client behavior to reflect the API and web client as closely as possible, so this makes sense. Do we have anyone from PLATFORM team coming to the demo on Friday? It might also be good to get more feedback from users on how valuable the .version support would be, as it's starting to look like a bigger undertaking than previously thought.

[jaymedina]

This test seems to be failing because there is already a File entity called foobarbat in Project syn12914772. I was able to re-create the error in my local machine after running syn.store on my File entity foobarbat consecutively:

After deleting my foobarbat file from my Project that I'm testing on, the syn.store call works fine again:

It looks like the Projects don't get deleted after integration tests run. I'm wondering why that is? I notice when I run integration tests locally the Projects remain in my Synapse Projects page indefinitely. Should we start an initiative to update tests so that we have complete teardowns after integration tests complate/fail, or is there a reason for Entities to stay up after the fact? @BryanFauble @thomasyu888

[jaymedina]

Another thing is this:

entity = syn.get("syn1234.3")
# entity.id = "syn1234"

The get API endpoint returns this object: https://rest-docs.synapse.org/rest/org/sagebionetworks/repo/model/FileEntity.html.

I'm thinking of handling this in two ways:

1. Start a new PR to fix syncToSynapse that excludes .version support for syncFromSynapse, syn.get and the CLI synapse get. This PR would be for the larger initiative to support .version across the whole codebase. I'm pretty sure I can patch up syncToSynapse without needing to change the others. The only thing I don't like about this is that I think syncTo/FromSynapse should always have very similar functionalities, since a user who uses one will probably use the other. And for .version to be supported in one and not the other is confusing.
2. Update the documentation letting people know that their input id may not be reflected in the output Entity object, and that the version is its own property. But this isn't ideal since I think it defeats the purpose of supporting .version notation in the input.

Aside from this, I think this PR is in a good state for a final review.

Let me know if there are other ideas @thomasyu888 @BryanFauble

[BryanFauble]

The get API endpoint returns this object: https://rest-docs.synapse.org/rest/org/sagebionetworks/repo/model/FileEntity.html.

I'm thinking of handling this in two ways:

1. Start a new PR to fix `syncToSynapse` that excludes .version support for `syncFromSynapse`, `syn.get` and the CLI `synapse get`. This PR would be for the larger initiative to support .version across the whole codebase. I'm pretty sure I can patch up `syncToSynapse` without needing to change the others. The only thing I don't like about this is that I think `syncTo/FromSynapse` should always have very similar functionalities, since a user who uses one will probably use the other. And for .version to be supported in one and not the other is confusing.
2. Update the documentation letting people know that their input `id` may not be reflected in the output Entity object, and that the `version` is its own property. But this isn't ideal since I think it defeats the purpose of supporting .version notation in the input.

I agree that it'll be confusing, but IMO we should let folks use .version notation for inputs (As in support it), but don't return that format. I'm thinking that we should always return back 1 expected object format from our APIs. By this I mean we have an ID field for syn123 and we have a versionNumber field denoting what version the entity is. I want those using the API to have confidence that all the information we're returning will always be consistent.

I also put together the following code to show explicit examples of what you are talking about with syn.get and what you are currently supporting. I am in agreement with how you implemented the changes.

import synapseclient
from synapseclient import Entity


syn = synapseclient.Synapse()
syn.login()

MY_ENTITY = "syn53369103"

# Works
by_id = syn.get(MY_ENTITY, downloadFile=False)
print(f"by_id: \n{by_id}")

# Works
by_entity = syn.get(Entity(id=MY_ENTITY), downloadFile=False)
print(f"by_entity: \n{by_entity}")

# Works
by_id_and_version = syn.get(f"{MY_ENTITY}.10", downloadFile=False)
print(f"by_id_and_version: \n{by_id_and_version}")

# Works - uses version 11 instead of 10
by_id_and_version_input_argument = syn.get(
    f"{MY_ENTITY}.10", version=11, downloadFile=False
)
print(f"by_id_and_version_input_argument: \n{by_id_and_version_input_argument}")

# Works
by_entity_and_version_field = syn.get(
    Entity(id=f"{MY_ENTITY}", versionNumber=10), downloadFile=False
)
print(f"by_entity_and_version_field: \n{by_entity_and_version_field}")

# Does not work
by_entity_and_version_in_id = syn.get(Entity(id=f"{MY_ENTITY}.10"), downloadFile=False)
print(f"by_entity_and_version_in_id: \n{by_entity_and_version_in_id}")

For a final version, should we support creating an Entity object like: Entity(id=f"{MY_ENTITY}.10")? Specifically I am thinking that we automatically split .10 from the ID and put it into the versionNumber field. Handle conflicts in the same way you are in .get().

[jaymedina]

For a final version, should we support creating an Entity object like: Entity(id=f"{MY_ENTITY}.10")? Specifically I am thinking that we automatically split .10 from the ID and put it into the versionNumber field. Handle conflicts in the same way you are in .get().

I think that makes sense, and yes I can recycle the logic that splits up the synid from the version number. Are you suggesting folding this into the PR as well?

[jaymedina]

We've agreed on how I implemented the changes for syn.get, synapse get (CLI), syncTo/FromSynapse + tests pass, and I've completed the checklist, so I'll mark this as RFR and continue addressing comments as they come. Thank you Bryan and Tom for your comments so far.

[BryanFauble]

For a final version, should we support creating an Entity object like: Entity(id=f"{MY_ENTITY}.10")? Specifically I am thinking that we automatically split .10 from the ID and put it into the versionNumber field. Handle conflicts in the same way you are in .get().

I think that makes sense, and yes I can recycle the logic that splits up the synid from the version number. Are you suggesting folding this into the PR as well?

@jaymedina It's an option, but I don't think it's required for us to support at this exact moment.

[jaymedina]

For a final version, should we support creating an Entity object like: Entity(id=f"{MY_ENTITY}.10")? Specifically I am thinking that we automatically split .10 from the ID and put it into the versionNumber field. Handle conflicts in the same way you are in .get().

I think that makes sense, and yes I can recycle the logic that splits up the synid from the version number. Are you suggesting folding this into the PR as well?

@jaymedina It's an option, but I don't think it's required for us to support at this exact moment.

I've added this item to the Jira ticket for broader .version support across synpy: https://sagebionetworks.jira.com/browse/SYNPY-699

[BryanFauble]

LGTM!

[jaymedina]

I need to leave this open for now because the syncFromSynapse and syn.get calls for synid.version input returns an empty etag for some reason. Need to investigate why.

[BryanFauble]

I need to leave this open for now because the syncFromSynapse and syn.get calls for synid.version input returns an empty etag for some reason. Need to investigate why.

@jaymedina I believe 00000s is the intended eTag when you are grabbing the non-latest version of an Entity.

[jaymedina]

That's right! Whew. Thanks! I'll go ahead and merge this now

[sonarcloud]

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

6 New issues
0 Security Hotspots
96.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[thomasyu888]

@jaymedina Just wanted to say this is great work and great reviews by @BryanFauble . I left some nits just for future reference, but no action required. Thanks everyone!

[thomasyu888]

Nit: in the future, when we see % lets switch to f-string interpolation.

[thomasyu888]

Nit: Just a thought for the future, but tuples can be a bit risky because the order really matters. Look into namedtuples for something more explicit.

[thomasyu888]

Nit: Not to over-engineer the solution, but in the future, these are technically different test cases. For example, right now it would be harder to determine if Test 2 or Test 3 failed, but if you split them into separate tests OR use pytest.parametrize (it it makes sense to) then it'd be easier to navigate.