Skip to content

Add storage_options to Snapshot StoragePlugin #108

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Add storage_options to Snapshot StoragePlugin #108

wants to merge 3 commits into from

Conversation

reyoung
Copy link
Contributor

@reyoung reyoung commented Oct 20, 2022

No description provided.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 20, 2022
@reyoung reyoung mentioned this pull request Oct 20, 2022
Open
@codecov
Copy link

codecov bot commented Oct 20, 2022 •
edited
Loading

Codecov Report

Merging #108 (d1d4df9) into main (cffb0cc) will decrease coverage by 4.79%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main     #108      +/-   ##
==========================================
- Coverage   91.80%   87.01%   -4.80%     
==========================================
  Files          31       31              
  Lines        2550     2556       +6     
==========================================
- Hits         2341     2224     -117     
- Misses        209      332     +123
Impacted Files Coverage Δ
torchsnapshot/storage_plugins/gcs.py 0.00% <0.00%> (-78.90%) ⬇️
torchsnapshot/storage_plugin.py 48.57% <80.00%> (-17.06%) ⬇️
torchsnapshot/snapshot.py 99.06% <100.00%> (+<0.01%) ⬆️
torchsnapshot/storage_plugins/fs.py 100.00% <100.00%> (ø)
torchsnapshot/storage_plugins/s3.py 26.82% <100.00%> (-63.18%) ⬇️
torchsnapshot/memoryview_stream.py 63.33% <0.00%> (-6.67%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ananthsub
ananthsub reviewed Oct 20, 2022
View reviewed changes
Copy link
Contributor

@ananthsub ananthsub left a comment

Choose a reason for hiding this comment

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

thanks for the contribution @reyoung !

reyoung
reyoung commented Oct 21, 2022
View reviewed changes
reyoung
reyoung commented Oct 21, 2022
View reviewed changes
yifuwang
yifuwang reviewed Oct 21, 2022
View reviewed changes
yifuwang
yifuwang approved these changes Oct 21, 2022
View reviewed changes
Copy link
Contributor

@yifuwang yifuwang left a comment

Choose a reason for hiding this comment

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

This look great! Thanks again @reyoung!

@ananthsub ananthsub mentioned this pull request Oct 21, 2022
Open
@reyoung
Copy link
Contributor Author

reyoung commented Oct 24, 2022

@yifuwang

Hello, it seems the CI reports code coverage check failed. However, this PR just adds a parameter to API and does not change any behavior. Can we skip the code coverage check?

@ananthsub
Copy link
Contributor

@reyoung unit tests are failing because storage_options is a required argument on the various StoragePlugin implementations. either the plugins should make storage_options optional, or all tests need to be explicitly pass in this option when initializing a storage plugin

@ananthsub ananthsub changed the title Add storage_kwargs to Snapshot.take Add storage_kwargs to Snapshot StoragePlugin Oct 25, 2022
@ananthsub ananthsub changed the title Add storage_kwargs to Snapshot StoragePlugin Add storage_options to Snapshot StoragePlugin Oct 25, 2022
@reyoung reyoung force-pushed the feature/storage_kwargs branch from 0adf39f to 54f749c Compare October 25, 2022 03:16
@reyoung reyoung force-pushed the feature/storage_kwargs branch from fbab921 to d1d4df9 Compare October 25, 2022 03:19
@reyoung
Copy link
Contributor Author

reyoung commented Oct 25, 2022

@ananthsub

Fixed. Just make storage_options optional in FSStoragePlugin/GCSStoragePlugin/S3StoragePlugin

@facebook-github-bot
Copy link
Contributor

@yifuwang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ananthsub ananthsub ananthsub left review comments

@yifuwang yifuwang yifuwang approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@reyoung @ananthsub @facebook-github-bot @yifuwang