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
Sky Data #63
Sky Data #63
Conversation
@romilbhardwaj @concretevitamin Ready to review! |
16ce4da
to
d24baa8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work! Left some comments, but I generally like how this has come out to be
prototype/sky/storage.py
Outdated
self.path = path | ||
assert not is_new_bucket or self.path | ||
if 'gcs://' not in self.path: | ||
if 's3://' in self.path: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we add more backends, we'll probably have to add more logic here for cross-cloud transfers.. In a future PR we should clean this up by adding something like a transfer registry which maps Tuple[Backend1, Backen2] -> TransferFn(Backend1, Backend2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great!! First pass, didn't look into all details but I tried to raise pressing high-level concerns/suggestions.
Question:
For GNN, I imagine they will always write
s = sky.Storage('my-data', source='/data/gnn/train')
# First run: upload.
# Second+ runs: sync (some overhead even if no change to data).
# This overhead is OK for now; future we can optimize it away.
s.to_s3()
task.set_storage_mounts({s: '/data/gnn/train'})
Is that right?
[Not for this PR] Suppose they now run this Sky script in a different machine without access to source='/data/gnn/train'
. We need a way (sky status
? sky storage
?) to enable them to find what S3 path is backing my-data
. So that they can write sky.Storage('my-data', source=<correct s3 path>)
instead.
Also, I just realized we will need to add support for |
Will be done in a followup PR after this is merged |
@romilbhardwaj @concretevitamin Think its good to go! Addressed 99% of the comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Mostly some more API ideas. Should be good to go after that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work! Left a minor comment.
Also, in a future PR we should investigate directly mounting the storage using GcsFuse/s3Fuse instead of copying to local EBS:
- Reduces setup time and EBS size (at the cost of increased disk latency/xput, need to evaluate this tradeoff)
- Allows for writes to Storage which users can access from other workers/local dev machine
@@ -12,7 +12,10 @@ | |||
import subprocess |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to team: As sky.Storage
becomes the standard method of uploading/interfacing with data in Sky, we should deprecate CloudStorage
and move it's functionality to AbstractStore
cc @concretevitamin @michaelzhiluo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, we can refactor in a later PR @romilbhardwaj @concretevitamin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outstanding work @michaelzhiluo !
Thanks for the reviews! @romilbhardwaj @concretevitamin |
* Initial Commit * Some more changes, mounting is left, finish tmrw * Demo * Fix * MVP is Done * ok * squashed commits * Yapf Fix * Romil's Comments Addressed * Partially addressed Zongheng's comments * Temporary fix * Fixed Resnet Storage example * Yapf and Pylint * Addressed Zongheng's comments, linting time * Pylint fix * Done
Completed Items
TODOs