-
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 1 commit
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
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,14 +1,19 @@ | ||||||
| # Sequences must be FASTA and metadata must be TSV | ||||||
| # Both files must be zstd compressed | ||||||
|
|
||||||
| # TODO XXX - remove commented lines below - only for testing the snakemake storage stuff | ||||||
| inputs: | ||||||
| - name: ncbi | ||||||
| metadata: "s3://nextstrain-data/files/workflows/zika/metadata.tsv.zst" | ||||||
| sequences: "s3://nextstrain-data/files/workflows/zika/sequences.fasta.zst" | ||||||
| # metadata: "https://data.nextstrain.org/files/workflows/zika/metadata.tsv.zst" | ||||||
| # sequences: "https://data.nextstrain.org/files/workflows/zika/sequences.fasta.zst" | ||||||
|
|
||||||
| additional_inputs: | ||||||
| - name: usvi | ||||||
| metadata: "data/metadata_usvi.tsv" | ||||||
| sequences: "data/sequences_usvi.fasta" | ||||||
| # metadata: https://raw.githubusercontent.com/nextstrain/zika/refs/heads/main/phylogenetic/data/metadata_usvi.tsv | ||||||
| # sequences: https://raw.githubusercontent.com/nextstrain/zika/refs/heads/main/phylogenetic/data/sequences.fasta | ||||||
|
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 surprised this didn't work, but in this case we can work around it by moving USVI data to a public s3 bucket (refer to it by http)
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. In case it helps, uploaded usvi to a public s3: Happy to drop 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. Seems like this was a case of typo resulting in unexpected error? (I would have expected a 404 error) Works fine with
Suggested change
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. Typo indeed! Ok that fixes it, and I've pushed up a WIP commit which we can fix up later on: cc86bd7. The error messaging around remote files (see main PR comment) doesn't seem ideal.
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 can you test out those files and see if you can reproduce the error i'm getting: $ curl -o metadata_usvi.tsv.zst https://data.nextstrain.org/files/workflows/zika/metadata_usvi.tsv.zst
$ zstd --decompress --stdout metadata_usvi.tsv.zst
[expected TSV output]
$ augur filter --metadata metadata_usvi.tsv.zst --output-metadata x
Traceback (most recent call last):
File "/Users/naboo/github/nextstrain/augur/augur/__init__.py", line 70, in run
return args.__command__.run(args)
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/naboo/github/nextstrain/augur/augur/filter/__init__.py", line 161, in run
return _run(args)
^^^^^^^^^^
File "/Users/naboo/github/nextstrain/augur/augur/filter/_run.py", line 198, in run
metadata_reader = read_metadata(
^^^^^^^^^^^^^^
File "/Users/naboo/github/nextstrain/augur/augur/io/metadata.py", line 96, in read_metadata
metadata = pd.read_csv(
^^^^^^^^^^^^
File "/Users/naboo/miniconda3/envs/augur-dev-snakemake-v9/lib/python3.12/site-packages/pandas/io/parsers/readers.py", line 1026, in read_csv
return _read(filepath_or_buffer, kwds)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/naboo/miniconda3/envs/augur-dev-snakemake-v9/lib/python3.12/site-packages/pandas/io/parsers/readers.py", line 620, in _read
parser = TextFileReader(filepath_or_buffer, **kwds)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/naboo/miniconda3/envs/augur-dev-snakemake-v9/lib/python3.12/site-packages/pandas/io/parsers/readers.py", line 1620, in __init__
self._engine = self._make_engine(f, self.engine)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/naboo/miniconda3/envs/augur-dev-snakemake-v9/lib/python3.12/site-packages/pandas/io/parsers/readers.py", line 1898, in _make_engine
return mapping[engine](f, **self.options)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/naboo/miniconda3/envs/augur-dev-snakemake-v9/lib/python3.12/site-packages/pandas/io/parsers/c_parser_wrapper.py", line 93, in __init__
self._reader = parsers.TextReader(src, **kwds)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "parsers.pyx", line 574, in pandas._libs.parsers.TextReader.__cinit__
File "parsers.pyx", line 663, in pandas._libs.parsers.TextReader._get_header
File "parsers.pyx", line 874, in pandas._libs.parsers.TextReader._tokenize_rows
File "parsers.pyx", line 891, in pandas._libs.parsers.TextReader._check_tokenize_status
File "parsers.pyx", line 2053, in pandas._libs.parsers.raise_parser_error
zstd.ZstdError: zstd decompress error: Unknown frame descriptor
An error occurred (see above) that has not been properly handled by Augur.
To report this, please open a new issue including the original command and the error above:
<https://github.com/nextstrain/augur/issues/new/choose>(I'm getting the same errors using Snakemake HTTP storage plugin, but the above
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. Huh, I'm getting the same error
Versions below, I'm using isolated conda runtime:
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. Okay, I have an idea what happened. I've uploaded new fixed Originally I had tried uploading via Realized that metadata_usvi.tsv had been zstd compressed during upload... geh I should have locally compressed and uploaded it directly
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.
Yeah, |
||||||
|
|
||||||
| # Config files | ||||||
| exclude: "exclude.txt" | ||||||
|
|
@@ -41,3 +46,4 @@ traits: | |||||
| - region | ||||||
| - country | ||||||
| sampling_bias_correction: 3 | ||||||
| # | ||||||
|
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.