[SYNPY-1356] Refactor sync to synapse #1083

BryanFauble · 2024-04-10T16:52:20Z

Problem:

The syncToSynapse function provided in the utility package had it's own multi-threaded implementation to allow the upload of files in parallel. Using this it had a shared thread pool executor with the file part upload as well.

Solution:

This PR contains the changes that were independently reviewed in these 2 PRs:
[SYNPY-1356] Moving file semaphore under file store #1084
[SYNPY-1356] Updating syncToSynapse file dependency saving logic #1089
Conversion of the multi-threaded portion of the syncToSynapse code to instead rely on asyncIO tasks/futures - Meaning all of the code is going to be executed in a single thread and only down the line will parts of files be uploaded in parallel on different threads.

Testing:

Planning to use the benchmark upload script in addition to integration testing around this logic.

pep8speaks · 2024-04-10T16:52:30Z

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

In the file synapseutils/monitor.py:

Line 149:89: E501 line too long (89 > 88 characters)

In the file synapseutils/sync.py:

Line 710:89: E501 line too long (89 > 88 characters)
Line 1193:89: E501 line too long (91 > 88 characters)

In the file tests/integration/synapseutils/test_synapseutils_sync.py:

Line 206:89: E501 line too long (117 > 88 characters)

Comment last updated at 2024-05-14 02:00:36 UTC

synapseutils/sync.py

* Add a global limit the number of files that can enter into uploading state

* Update saving of files to use AsyncIO and tasks

thomasyu888 · 2024-05-14T00:52:16Z

synapseclient/models/file.py

@@ -814,6 +816,7 @@ async def change_metadata_async(
        """
        if not self.id:
            raise ValueError("The file must have an ID to change metadata.")
+        from synapseutils.copy_functions import changeFileMetaData


For future PR: Should these just be internal client functions instead of synapseutils?

thomasyu888

🔥 Nice! The bulk of the reviews came through the other PRs.

thomasyu888 · 2024-05-14T01:02:56Z

synapseclient/core/upload/multipart_upload_async.py

+
+
+@contextmanager
+def shared_progress_bar(progress_bar):


Will this only be used in the multi part upload?

For now - Yes. I may extract this with changing how downloads are working.

* Cleaning up code and provding benchmarking results

BryanFauble added 30 commits March 7, 2024 11:49

Adding an async client and a few APIs that use it

0d5f8a9

New dependency cache

6d000a4

Adding unit/integration tests around logic

1675486

More type hinting

24a9cd7

Correct example

3e7e304

Correct typing

bf39455

Use content field instead of deprecated data field

a0434e9

create event loops for integration tests

74014d2

Sonarcloud resolutions

0284c60

Unit test corrections

999508e

Adding mkdocs around changes

f4c48a5

Code review feedback around exception handling

c5b5ca2

Corrections around integration testing

6130dc2

Minor fixes to try excepts

708a9a8

First pass for async file uploads

82af985

Testing a mix of multi-threaded and async

23c34cb

Run in executor

5ae4a0d

Pass along max threads

afc59bb

run with executor around upload_parts

040e59a

Tweaks to executor

6706d16

Complete part upload outside of threadpool

9811397

Execute md5 calculation in threadpool

7f4a243

Execute md5 check in process pool

c30d57e

Wrap in with context

52d44d2

Execute part completion concurrently with other part uploads

d7e04de

Cleaning up some code

a8bb94d

Corrections to let threads finish upload before failing/retrying upload

e5dbef8

Bug fixs around file upload and storage. Adding TQDM progress bar

44b4fa8

Merge branch 'develop' into SYNPY-1420-async-upload-multi-threaded-mix

7257be7

Close progress bar on complete

a6ddf25

BryanFauble added 13 commits April 8, 2024 16:47

Return response correctly

121ba68

Add more logging around retry logic

3c52231

a

fe3da5a

a

428b2bd

Merge branch 'develop' into SYNPY-1456-flaky-integration-tests

762a28a

Cleaning up some temporary code

1a6bcd3

Cleaning up test code

3311e5f

Leave OTEL during int tests disabled by default

5b99ea9

Include exception information in retry failure debug message

ca618da

Include exc_info in debug failure

2621f3b

Turn off port forwading in int test

fdedae3

Ignore if no files found

7d27c6b

Bare minimum to migrate sync-to-synapse to use asyncio

d31a4c1

BWMac reviewed Apr 10, 2024

View reviewed changes

synapseutils/sync.py Outdated Show resolved Hide resolved

synapseutils/sync.py Outdated Show resolved Hide resolved

synapseutils/sync.py Outdated Show resolved Hide resolved

synapseutils/sync.py Outdated Show resolved Hide resolved

synapseutils/sync.py Outdated Show resolved Hide resolved

Code review feedback

699c015

Base automatically changed from SYNPY-1456-flaky-integration-tests to develop April 10, 2024 23:21

Merge branch 'develop' into SYNPY-1356-refactor-sync-to-synapse

d85afe5

jaymedina reviewed Apr 16, 2024

View reviewed changes

synapseutils/sync.py Outdated Show resolved Hide resolved

BryanFauble added 3 commits April 23, 2024 08:19

[SYNPY-1356] Moving file semaphore under file store (#1084)

80a5f06

* Add a global limit the number of files that can enter into uploading state

Merge branch 'develop' into SYNPY-1356-refactor-sync-to-synapse

018cee6

[SYNPY-1356] Updating syncToSynapse file dependency saving logic (#1089)

1bb3725

* Update saving of files to use AsyncIO and tasks

BryanFauble marked this pull request as ready for review May 13, 2024 17:27

BryanFauble requested a review from a team as a code owner May 13, 2024 17:27

BryanFauble changed the title ~~DRAFT: [SYNPY-1356] Refactor sync to synapse~~ [SYNPY-1356] Refactor sync to synapse May 13, 2024

thomasyu888 reviewed May 14, 2024

View reviewed changes

thomasyu888 approved these changes May 14, 2024

View reviewed changes

[SYNPY-1356] Cleaning up code and provding benchmarking results (#1097)

7ea004d

* Cleaning up code and provding benchmarking results

BryanFauble merged commit be9f166 into develop May 14, 2024
1 of 3 checks passed

BryanFauble deleted the SYNPY-1356-refactor-sync-to-synapse branch May 14, 2024 02:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYNPY-1356] Refactor sync to synapse #1083

[SYNPY-1356] Refactor sync to synapse #1083

BryanFauble commented Apr 10, 2024 •

edited

pep8speaks commented Apr 10, 2024 •

edited

thomasyu888 May 14, 2024 •

edited

thomasyu888 left a comment •

edited

thomasyu888 May 14, 2024

BryanFauble May 14, 2024

[SYNPY-1356] Refactor sync to synapse #1083

[SYNPY-1356] Refactor sync to synapse #1083

Conversation

BryanFauble commented Apr 10, 2024 • edited

pep8speaks commented Apr 10, 2024 • edited

Comment last updated at 2024-05-14 02:00:36 UTC

thomasyu888 May 14, 2024 • edited

Choose a reason for hiding this comment

thomasyu888 left a comment • edited

Choose a reason for hiding this comment

thomasyu888 May 14, 2024

Choose a reason for hiding this comment

BryanFauble May 14, 2024

Choose a reason for hiding this comment

BryanFauble commented Apr 10, 2024 •

edited

pep8speaks commented Apr 10, 2024 •

edited

thomasyu888 May 14, 2024 •

edited

thomasyu888 left a comment •

edited