-
Notifications
You must be signed in to change notification settings - Fork 11
WIP - snakemake v8+ remote storage support #89
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
Changes from 2 commits
26ad6e9
cc86bd7
70e16f8
9168fc6
879ba99
79ecf00
5319043
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
joverlee521 marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,82 @@ | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| Helper functions to set-up storage plugins for remote inputs/outputs. | ||||||||||||||||||||||
| See the docstring of `path_or_url` for usage instructions. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| <https://snakemake.readthedocs.io/en/stable/snakefiles/storage.html> | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| from urllib.parse import urlparse | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| PUBLIC_BUCKETS = set(['nextstrain-data']) # TODO XXX | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| _storage_registry = {} # keeps track of registered storage plugins to enable reuse | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def _storage_s3(*, bucket, keep_local, force_signed): | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| Returns a Snakemake storage plugin for S3 endpoints | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| retries=2 # num S3 retries attempted | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # If the bucket is public then we make an unsigned request (i.e. no AWS credentials | ||||||||||||||||||||||
| # need to be set). Note that this won't work for uploads. | ||||||||||||||||||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should drop this special-treatment of known public buckets? It would simplify the code and let us avoid the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm open to public buckets being referred to by the https: URLs. And then keeping s3 handling for less public (private? restricted use?) datasets in case that usecase shows up if it hasn't already.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for removing the special treatment for public buckets. I say follow our own guidance from ncov/remote_inputs docs:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I'd say that advice is conditioned on the situation where we have to be authenticated for public bucket requests. But this PR shows we don't. So if we pay a small code-complexity price we can get cheaper & faster transfers (S3) without the frustrating UX of needing to provide credentials. If it were only data downloading I'd keep this PR as is, it was only when testing/considering uploads that I started to question it. I'll think about this some more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the misleading errors around credentials, should we enforce credential requirements here, e.g.:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated the code and I'm happy with the UX now. Here's the logic of when we use signed vs unsigned:
and a table of situations:
Error 1: Error 2:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for thinking this through @jameshadfield, I really like the outlined behavior! Providing bad credentials produce slightly different errors, but they should be informative for the user: |
||||||||||||||||||||||
| if not force_signed and bucket in PUBLIC_BUCKETS: | ||||||||||||||||||||||
| if provider:=_storage_registry.get('s3_unsigned', None): | ||||||||||||||||||||||
| return provider | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| from botocore import UNSIGNED | ||||||||||||||||||||||
| storage s3_unsigned: | ||||||||||||||||||||||
| provider="s3", | ||||||||||||||||||||||
| signature_version=UNSIGNED, | ||||||||||||||||||||||
| retries=retries, | ||||||||||||||||||||||
| keep_local=keep_local, | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| _storage_registry['s3_unsigned'] = storage.s3_unsigned | ||||||||||||||||||||||
| return _storage_registry['s3_unsigned'] | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # Default: resource fetched via a signed request, which will require AWS credentials | ||||||||||||||||||||||
| if provider:=_storage_registry.get('s3_signed', None): | ||||||||||||||||||||||
| return provider | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # the tag appears in the local file path, so reference 'signed' to give a hint about credential errors | ||||||||||||||||||||||
| storage s3_signed: | ||||||||||||||||||||||
| provider="s3", | ||||||||||||||||||||||
| retries=retries, | ||||||||||||||||||||||
| keep_local=keep_local, | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| _storage_registry['s3_signed'] = storage.s3_signed | ||||||||||||||||||||||
| return _storage_registry['s3_signed'] | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def _storage_http(*, keep_local): | ||||||||||||||||||||||
| if provider:=_storage_registry.get('http', None): | ||||||||||||||||||||||
| return provider | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| storage: | ||||||||||||||||||||||
| provider="http", | ||||||||||||||||||||||
| allow_redirects=True, | ||||||||||||||||||||||
| supports_head=True, | ||||||||||||||||||||||
| keep_local=keep_local, | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| _storage_registry['http'] = storage.http | ||||||||||||||||||||||
| return _storage_registry['http'] | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def path_or_url(uri, *, keep_local=True, force_signed=False): | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| Returns the URI wrapped by an applicable storage plugin. | ||||||||||||||||||||||
| Local filepaths will be returned unchanged. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| TODO XXX - document usage more thoroughly | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| info = urlparse(uri) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if info.scheme=='': # local | ||||||||||||||||||||||
| return uri # no storage wrapper | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if info.scheme=='s3': | ||||||||||||||||||||||
| return _storage_s3(bucket=info.netloc, keep_local=keep_local, force_signed=force_signed)(uri) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if info.scheme in ['http', 'https']: | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to support plain HTTP? Requiring HTTPS doesn't feel onerous and is kind of a security best-practice…
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's currently supported in ncov so I think we're trying to keep the functionality the same here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...ok, but -- is it actually used in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True! Especially since the Snakemake upgrade is a breaking change anyways...
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||||||||||||
| return _storage_http(keep_local=keep_local)(uri) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # TODO XXX - Google? We allowed this in ncov <https://github.com/nextstrain/ncov/blob/41cf6470d3140963ff3e02c29241f80ae8ed9c33/workflow/snakemake_rules/remote_files.smk#L62> | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for supporting GS urls if possible (e.g. Terra users, etc)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1, I've included the GS plugin (
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good - can one of you add it to this PR?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added in 70e16f8, but not sure how to test here...
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @j23414 are you able to test the GS stuff? I presume Broad is mirroring the zika files?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, seems like Broad is mirroring everything in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @j23414 and I discussed GS in person yesterday. We ran into authentication errors that indicates you need to set up Application Default Credentials in order to use the GS plugin. I have not found any way of passing in credentials via envvars for this plugin.... Even the old Snakemake v7 GS provider required you to login via All that is to say, I'm walking back on my support for GS and think it's not worth the trouble.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for testing! I'm going to take this code over to ncov now and will remove GS. I'll add a specific conditional to catch the GS scheme (and HTTP) and raise an error telling users to get in touch with us because we can add those functionality if pushed to do so.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| raise Exception(f"Input address {uri!r} (scheme={info.scheme!r}) is from a non-supported remote") | ||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.