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

[WIP] Use WebSockets for uploading/downloading files and directories (using callbacks). #954

Closed
wants to merge 22 commits into from

Conversation

serhiy-storchaka
Copy link
Contributor

Closes #929.

@serhiy-storchaka serhiy-storchaka added the enhancement New feature or request label Aug 13, 2019
Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Looks good, the design is clear and readable.
Please go ahead.

@lgtm-com
Copy link

lgtm-com bot commented Aug 13, 2019

This pull request introduces 1 alert when merging eacd820 into 8a55413 - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Aug 13, 2019

This pull request fixes 7 alerts when merging 9d8288c into 8a55413 - view on LGTM.com

fixed alerts:

  • 4 for Module is imported with 'import' and 'import from'
  • 3 for Conflicting attributes in base classes

@serhiy-storchaka
Copy link
Contributor Author

Tests are failed because the API is tested with a small builtin storage server, which just does not support WebSocket operations yet.

@lgtm-com
Copy link

lgtm-com bot commented Aug 14, 2019

This pull request introduces 2 alerts when merging 1e96567 into 6089723 - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Aug 14, 2019

This pull request introduces 1 alert when merging 7a2ac0b into cf1a4ca - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Aug 14, 2019

This pull request fixes 7 alerts when merging 4a2fe81 into cf1a4ca - view on LGTM.com

fixed alerts:

  • 4 for Module is imported with 'import' and 'import from'
  • 3 for Conflicting attributes in base classes

@codecov
Copy link

codecov bot commented Aug 15, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@078fd59). Click here to learn what that means.
The diff coverage is 86.46%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #954   +/-   ##
=========================================
  Coverage          ?   91.69%           
=========================================
  Files             ?       37           
  Lines             ?     4518           
  Branches          ?      737           
=========================================
  Hits              ?     4143           
  Misses            ?      254           
  Partials          ?      121
Impacted Files Coverage Δ
neuromation/api/storage.py 93.47% <86.46%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 078fd59...57ee5cc. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 15, 2019

Codecov Report

Merging #954 into master will decrease coverage by 1.52%.
The diff coverage is 87.8%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #954      +/-   ##
==========================================
- Coverage   92.09%   90.57%   -1.53%     
==========================================
  Files          37       37              
  Lines        4251     4635     +384     
  Branches      642      728      +86     
==========================================
+ Hits         3915     4198     +283     
- Misses        227      321      +94     
- Partials      109      116       +7
Impacted Files Coverage Δ
neuromation/api/storage.py 93.36% <87.8%> (-4.9%) ⬇️
neuromation/cli/job.py 76.28% <0%> (-9.1%) ⬇️
neuromation/api/jobs.py 93.49% <0%> (-2.68%) ⬇️
neuromation/cli/main.py 68.31% <0%> (-1.49%) ⬇️
neuromation/cli/utils.py 89.29% <0%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 078fd59...91b00bf. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Aug 15, 2019

This pull request introduces 1 alert when merging 57ee5cc into 9d6c96f - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

@serhiy-storchaka serhiy-storchaka changed the title Use WebSockets for uploading/downloading files and directories. [WIP] Use WebSockets for uploading/downloading files and directories. Aug 16, 2019
@serhiy-storchaka
Copy link
Contributor Author

Some benchmarking results.

Master:
39-45 seconds for neuro storage cp -r neuromation storage:
32-45 seconds for neuro storage cp -r storage:neuromation /tmp
7 seconds for neuro storage cp neuromation.tar storage:
2 seconds for neuro storage cp storage:neuromation.tar /tmp

Current PR:
7-8 seconds for neuro storage cp -r neuromation storage:
3-4 seconds for neuro storage cp -r storage:neuromation /tmp
8 seconds for neuro storage cp neuromation.tar storage:
3 seconds for neuro storage cp storage:neuromation.tar /tmp

See also #958.

@lgtm-com
Copy link

lgtm-com bot commented Aug 16, 2019

This pull request introduces 1 alert when merging 8482ac5 into 5bf8ff6 - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

@asvetlov
Copy link
Contributor

Super impressive numbers!

@lgtm-com
Copy link

lgtm-com bot commented Aug 16, 2019

This pull request introduces 1 alert when merging a9a188c into 5bf8ff6 - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Aug 16, 2019

This pull request introduces 1 alert when merging ffbb7bf into 4548126 - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Aug 16, 2019

This pull request introduces 1 alert when merging 1aa7b68 into f5148b9 - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Aug 16, 2019

This pull request introduces 1 alert when merging 8445153 into f5148b9 - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Sorry, I don't follow when WS client shuts down the connection after sending all data?
Who calls "await ws.close()"?

Using async generator handlers is wise BTW

exc: BaseException
if "errno" in payload:
exc = OSError(
errno.__dict__.get(payload["errno"], payload["errno"]),
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I don't follow: is payload["errno"] an int or str abbreviation?

@asvetlov
Copy link
Contributor

Aha, I see if not self._handlers: break in run().

@lgtm-com
Copy link

lgtm-com bot commented Aug 19, 2019

This pull request introduces 5 alerts when merging 8452b14 into a2a1cb7 - view on LGTM.com

new alerts:

  • 4 for Unused local variable
  • 1 for Module is imported with 'import' and 'import from'

@serhiy-storchaka serhiy-storchaka changed the title [WIP] Use WebSockets for uploading/downloading files and directories. [WIP] Use WebSockets for uploading/downloading files and directories (using callbacks). Aug 22, 2019
@asvetlov asvetlov deleted the storage-websocket branch November 4, 2020 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support bulk load in neuro cp
3 participants