-
Notifications
You must be signed in to change notification settings - Fork 415
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
Storage CLI #121
Storage CLI #121
Conversation
@infwinston @Michaelvll would you guys look over the API and see if it's natural for your use case? |
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 great! I addressed some nits and added some documentation to the yaml in new commits, please feel free to review those before merging
if cloud_type in self.stores: | ||
logger.info(f'Storage type {cloud_type} already exists!') | ||
return self.stores[cloud_type] |
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.
Some thoughts, no action needed:
This seems fine (one storage object per cloud), but I wonder if there are any use-cases where a single storage object can have multiples stores of the same cloud_type. Perhaps stores in different regions but on the same cloud? For now, this is okay.
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.
Yes, in the future the StorageEnum class will change to include both CLOUD_TYPE.REGION_NAME. Good point!
@@ -426,7 +424,7 @@ def _upload_file(self, local_file: str, remote_path: str) -> None: | |||
remote_path: str; Remote path on GCS bucket | |||
""" | |||
blob = self.bucket.blob(remote_path) | |||
blob.upload_from_filename(local_file) | |||
blob.upload_from_filename(local_file, timeout=None) |
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.
I assume these are GCS-specific details (times out on uploading large files?)
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.
Yes, it broke upon uploading huge files (timeout=60s). This seemed to fix it
@romilbhardwaj This LGTM. Thanks for the detailed comments in |
resnet-model-dir: 0.1, | ||
} | ||
|
||
# storage: List[sky.Storage] |
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.
@infwinston @Michaelvll This should make storage clear for the GNN use case!!!
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!
* Storage CLI Init Commit * Bug found #1 * Deep Copy Fix + other bugs * nits, docs * lint * Add docs for storage.name * doc fixes * doc fixes * doc fixes * fix docs for aws sync --delete flag * Fix Co-authored-by: Romil <romil.bhardwaj@gmail.com>
Adds Storage CLI.
Example: (also in
examples/resnet_app_storage.yaml
)