[SYNPY-1800] Added new sync_to_synapse method#1353
Conversation
linglp
left a comment
There was a problem hiding this comment.
Old syncToSynapse defaults to sendMessages=True, and new sync_to_synapse defaults to send_messages=False. Users migrating by following the deprecation example will silently stop receiving upload notifications with no warning. Is this behavior intentional?
Also, _create_local_test_file leaves cleanup to the caller. I think it will be better if clean up can happen within _create_local_test_file so that we could avoid all the scattered schedule_for_cleanup calls.
linglp
left a comment
There was a problem hiding this comment.
Hi @andrewelamb ! I found more problems with the documentation. Please see comments below
linglp
left a comment
There was a problem hiding this comment.
@andrewelamb Thanks for the last round of fixes. there are still some problems with tests and documentation. Could you check again?
| Using multiple values for a single annotation should be used sparingly as it makes it more | ||
| difficult for you to manage the data. However, it is supported. | ||
|
|
||
| **Annotations can be comma `,` separated lists surrounded by brackets `[]`.** |
There was a problem hiding this comment.
Take a look at the old documentation here: https://python-docs.synapse.org/en/stable/explanations/manifest_tsv/?h=manifest#multiple-values-of-annotations-per-key, the examples here are much shorter. Is this epxected?
There was a problem hiding this comment.
Yes, I wasn't sure all the examples needed to be included. What do you think?
| is_synapse_id_str, | ||
| is_url, | ||
| test_import_pandas, | ||
| topolgical_sort, |
There was a problem hiding this comment.
probably this needs to be fixed in a different PR. Is there a typo in the function name here? should this be topological_sort instead of topolgical_sort?
There was a problem hiding this comment.
That is correct. We would need to decide if this is a public function, and if so it would need to be deprecated first.
| dep_file = _make_file_mock("/dep.txt", "syn_dep") | ||
|
|
||
| # Create a real future that resolves to dep_file | ||
| dep_future = asyncio.get_event_loop().create_future() |
There was a problem hiding this comment.
should we be using asyncio.get_event_loop() here? Based on the documentation, for Python 3.10+, we should consider using asyncio.run
There was a problem hiding this comment.
So Claude says on this :
In this context — an async test function — neither asyncio.get_event_loop() nor asyncio.run() is the right choice.
Since the test is already running inside an async event loop (managed by pytest-asyncio), the correct approach is:
dep_future = asyncio.get_running_loop().create_future()
- get_event_loop() is deprecated for this use case in 3.10+ and emits a deprecation warning when no running loop exists.
- asyncio.run() would try to create a new event loop, which conflicts with the already-running one.
- get_running_loop() is the correct call from within an async function — it returns the loop that's already executing the test.
There was a problem hiding this comment.
There are still some problems with tests and documentation. One concern that I have is that a lot of tests are relying on sync_from_synapse_async for clean up which means if the assertion failed after sync_from_synapse_async or the function itself failed, the test files won't be cleaned up at all. But overall it looks good!
Problem:
The
syncToSynapsefunction doesn't fit the new OOP model paradigm.Solution:
sync_to_synapsemethod was added to theStorableContainerclasssyncToSynapsewas deprecatedTesting:
sync_to_synapsemethod