search

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] Moving file semaphore under file store #1084

Closed BryanFauble closed 2 months ago

BryanFauble commented 2 months ago

Problem:

  1. The sync util function had a mechanism to limit the number of files that can enter the store process.

Solution:

  1. Adding a semaphore to the client.
  2. Moving code from the sync function into the File.store_async() function that wraps the upload method. Now only a limited amount of files can enter into the "uploading" state.

Testing: I tested out storing a few different data sizes. 1000/1GiB: 125.61s 1/10GiB: 170.75s 100/10GiB: 156.92s

These numbers are in-line with the bench marked numbers collected on the table here: https://github.com/Sage-Bionetworks/synapsePythonClient/blob/620937a91398a2446b0483619ae88a15adede3f8/docs/explanations/benchmarking.md#some-insights

pep8speaks commented 2 months ago

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

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

BryanFauble commented 2 months ago

🔥 LGTM! This looks great minus the failing tests.

Also - are we not executing sonarcloud currently - or am I just missing it somehow...

@thomasyu888 Sonar is setup to run after the tests successfully execute since only then do we have the coverage report. However, you bring up a good point - we should let sonar run even on failure. Would you mind creating a tech debt synpy ticket for me for this?