imp: adds helpers to get metadata/artifacts from URLs in usage examples #658

gregcaporaso · 2022-10-24T19:14:57Z

Addresses #657

Still needs:

doc strings
unit tests
error detection and reporting on failed downloads (e.g., report 404 with the URL rather than failing when trying to load nonsense data as Metadata)
~~init_metadata_column_from_url~~ ~~this is hooked up by passing column to init_metadata_from_url~~ going to skip this in this PR altogether
update handling of integration of {epoch} in URLs to replace with the most recent epoch
is this encoding/replacement of {epoch} a reasonable way to handle this? we decided to use f-strings instead, based on feedback from @ebolyen
address new TODOs throughout

Submitting a draft PR so we can use it in our usage sprint example.

gregcaporaso · 2022-10-24T20:26:42Z

This can be used as follows:

def feature_table_filter_samples(use):
    feature_table = use.init_artifact_from_url(
        'feature_table',
        'https://docs.qiime2.org/{epoch}/data/tutorials/moving-pictures/table.qza'
        )
    sample_metadata = use.init_metadata_from_url(
        'sample_metadata',
        'https://data.qiime2.org/{epoch}/tutorials/moving-pictures/sample_metadata.tsv'
        )

    filtered_table, = use.action(
        use.UsageAction(plugin_id='feature_table', action_id='filter_samples'),
        use.UsageInputs(table=feature_table, metadata=sample_metadata,
                        where="[body-site] IN ('left palm', 'right palm')"),
        use.UsageOutputNames(filtered_table='filtered_table')
    )

lizgehret

This great, thanks for adding this @gregcaporaso! Everything looks reasonable to me, and it seems like you have good test coverage for the common use-cases.

Let's let @ebolyen take a final look over this one before it's merged (in case there's any missing context on something that could cause problems).

lizgehret · 2022-10-25T17:13:50Z

qiime2/sdk/usage.py

+        # Obtaining epoch modeled on qiime2.metadata.io.MetadataFileError
+        import qiime2
+
+        epoch = qiime2.__release__


I like this approach!

lizgehret · 2022-10-25T17:22:31Z

qiime2/sdk/usage.py

+        return data
+
+    def init_artifact_from_url(self, name: str, url: str,
+                               replace_url_epoch: bool = True


I like this too - on the off-chance that we ever don't need the epoch in a given artifact URL, this will be helpful.

lizgehret · 2022-10-25T17:26:35Z

qiime2/sdk/usage.py

+
+        return self.init_artifact(name, factory)
+
+    def init_metadata_from_url(self, name: str, url: str, column: str = None,


Love the addition of the column parameter, this is so helpful!

lizgehret · 2022-10-25T17:34:22Z

qiime2/sdk/usage.py

@@ -922,6 +922,146 @@ def init_format(self, name: str,
        """
        return self._usage_variable(name, factory, 'format')

+    def _replace_url_epoch(self, url):
+        # Obtaining epoch modeled on qiime2.metadata.io.MetadataFileError
+        import qiime2


Suggested change

import qiime2

qiime2 is being imported at the top of this file, so it shouldn't be needed as an additional import here.

lizgehret · 2022-10-25T17:36:15Z

qiime2/sdk/usage.py

+        return url.replace('{epoch}', epoch)
+
+    def _request_url(self, url):
+        import requests


It's interesting - the requests module isn't being found when CI is running your tests. I'm wondering if it actually does need to be added at the top of the file, or maybe in the recipe file as well.

Oh, I wonder if it's not actually a framework dependency?

I might recommend using stdlib for this (even if it's slightly unpleasant), since there's not too much going on. If we were doing lots of URL requests, then a 3rd party lib might be a good choice.

Since almost everyone uses requests, it's easy for us to end up in a dependency bind someday, so not using it would avoid that.

lizgehret

Added a couple of additional comments inline re: CI failures.

ebolyen · 2022-10-25T19:23:37Z

Big picture questions:

These seems like the first example of "utility" methods. It seems these are not generally expected to be overridden by each interface. It occurs to me that it would be perfectly valid for them to do so however. Are we cool with that? It seems reasonable enough.
I think with python's f-strings we might be able to not worry about the epoch replacement, is it very much work to do:
```
def example(use):
    import qiime2
    use.init_artifact_from_url(f'http://whatever.org/{qiime2.__release__}/foo.qza')
```
This avoids any unanticipated behavior, and we should be able to consider __release__ to be stable. Otherwise, I think the default should probably be False. The epoch replacement parameter also makes implementing interface overriding more complicated, as they would all need some mechanism to repeat the epoch logic.
The column parameter on init_metadata_from_url is a pretty good idea, however will the lack of friction cause any confusion when relating it to other methods? Said differently, is the current "syntactic salt" of having to split the two operations beneficial as it requires acknowledging the difference, or should we go the other way and also add use.init_metadata_column which is a similar utility method? If that happened, should we have init_metadata_column_from_url to keep things clear?
- An alternative would be to add column to init_metadata as well, but this would break the API for each interface which implements those, so I don't think we want to do that.

gregcaporaso · 2022-10-25T22:40:50Z

Thanks for the feedback @lizgehret and @ebolyen!

@ebolyen:
| use.init_artifact_from_url(f'http://whatever.org/{qiime2.__epoch__}/foo.qza')

Did you mean qiime2.__release__ here? qiime2.__epoch__ doesn't appear to be defined.

ebolyen · 2022-10-25T22:43:14Z

Oh yeah, sorry. That's what I get for doing quick drive-by comments.

gregcaporaso · 2022-10-26T00:04:18Z

@lizgehret, @ebolyen - I partially addressed the comments here.

the requests module isn't being found
I might recommend using stdlib

Updated to use urllib instead of requests.

These seems like the first example of "utility" methods. It seems these are not generally expected to be overridden by each interface. It occurs to me that it would be perfectly valid for them to do so however. Are we cool with that? It seems reasonable enough.

I agree - generally don't expect them to be overridden, but I don't think there is harm if they are.

I think with python's f-strings...

That makes sense to me, and I implemented this change. For folks using the previous version from this PR, you would change something like this:

metadata_url = \
            'https://data.qiime2.org/{epoch}/tutorials/moving-pictures/sample_metadata.tsv'
use.init_metadata_from_url('md', metadata_url)

to something like this:

import qiime2
metadata_url = \
            f'https://data.qiime2.org/{qiime2.__release__}/tutorials/moving-pictures/sample_metadata.tsv'
use.init_metadata_from_url('md', metadata_url)

The column parameter on init_metadata_from_url is a pretty good idea, however...

I think you're right that we shouldn't take this approach. Something I didn't notice until today is that the var_type on the result was incorrect in the current implementation in this PR (it's metadata when it should be column).

Going with use.init_metadata_column and use.init_metadata_column_from_url seems to make the most sense to me. I spent some time on this this afternoon but am stuck without working code at the moment - not sure what I'm doing wrong. I probably won't have time to get back to this till late this week - if someone else wants to take a crack and commit to this PR that's fine with me. Here's a diff of the code that I have locally right now where this is not working.

gregcaporaso · 2022-10-26T02:02:06Z

qiime2/sdk/usage.py

+        Examples
+        --------
+        >>> import qiime2
+        >>> url = f'https://data.qiime2.org/{qiime2.__release__}/tutorials/moving-pictures/sample_metadata.tsv' # noqa: E501


I can't figure out how to get a multi-line string working as input in a doc string. As it stands, I'm disabling the max line length check for flake8. Open to other ideas though.

I think this will do what you want. Assuming I remembered how to show continued expressions in doctest.

Suggested change

>>> url = f'https://data.qiime2.org/{qiime2.__release__}/tutorials/moving-pictures/sample_metadata.tsv' # noqa: E501

>>> url = (f'https://data.qiime2.org/{qiime2.__release__}/tutorials/'

... 'moving-pictures/sample_metadata.tsv')

I tested @ebolyen's suggestion locally, and that worked for me!

I could have sworn I tried that but I guess not! Or maybe something unrelated was failing when I tried and mistook it for this failing. In any case, thank you both!

gregcaporaso · 2022-10-28T16:34:45Z

I decided to drop the column selection support from this PR for now as I was continuing to run into trouble with it as I developed unit tests and real-world examples and I don't want to hold this PR any more due to this. So, if you were previously doing something like:

md = use.init_metadata_from_url('md', metadata_url, column='body-site')

You should now do:

column = 'body-site'
md = use.init_metadata_from_url('md', metadata_url)
mdc = use.get_metadata_column('mdc', column, md)

lizgehret · 2022-11-01T21:49:30Z

These changes look great @gregcaporaso! Excited to have this available for usage examples moving forward. Unless @ebolyen has any change requests, this looks g2g 👍

lizgehret · 2022-11-17T19:11:25Z

Oh hmm that's interesting @Oddant1 - I wonder if this needs to be __epoch__ instead of __release__. I'll take a look at this later today and see if I get the same result as you do!

lizgehret · 2022-11-17T21:03:09Z

So after a make dev on my machine, this is the QIIME 2 version that I'm seeing:

QIIME 2 version: 2021.8.0.dev0+68.g1339294

@Oddant1 what QIIME 2 release is your conda env created from? I'm wondering if that is (at least part of) the inconsistency here. --> Yeah it looks like this is related to the conda env being used - I'm not sure if it's pulled from anything else, so I could totally be missing something.

Oddant1 · 2022-11-17T22:31:03Z

@lizgehret I figured it out and deleted my comment (probably shoulda just edited it). I just needed to do git fetch --all --tags

ebolyen

Thanks for the reviews everyone!

gregcaporaso added 2 commits October 24, 2022 12:13

adds helpers to get metadata and artifacts from URLs in usage examples

0e2da6c

addresses qiime2#657

lint

457b090

gregcaporaso added 3 commits October 24, 2022 15:12

add ability to not encode epoch in urls

9f1375b

lint

2bc4596

enable getting a Metadata column from url

c40124d

ChrisKeefe mentioned this pull request Oct 25, 2022

ex: Adds usage examples for Hacktoberfest qiime2/q2-deblur#93

Merged

gregcaporaso added 3 commits October 25, 2022 07:22

added unit tests

7525995

add doc strings

217d75d

improve discovery of epoch for url replacement

474dcda

gregcaporaso requested review from lizgehret and ebolyen October 25, 2022 17:07

lizgehret approved these changes Oct 25, 2022

View reviewed changes

lizgehret reviewed Oct 25, 2022

View reviewed changes

transitioned requests to urllib

272b842

gregcaporaso added 2 commits October 25, 2022 16:02

epoch handling now uses F-strings

b6c321d

squash

ccd2436

gregcaporaso commented Oct 26, 2022

View reviewed changes

ebolyen added the hacktoberfest-accepted label Oct 26, 2022

dropped the column-getter from init_metadata_from_url

1339294

gregcaporaso marked this pull request as ready for review November 1, 2022 20:46

lizgehret linked an issue Nov 1, 2022 that may be closed by this pull request

imp: support for obtaining artifacts/metadata from URLs in usage examples #657

Closed

lizgehret changed the title ~~adds helpers to get metadata and artifacts from URLs in usage examples~~ imp: adds helpers to get metadata/artifacts from URLs in usage examples Nov 1, 2022

ebolyen approved these changes Nov 22, 2022

View reviewed changes

ebolyen merged commit 77df12a into qiime2:master Nov 22, 2022

gregcaporaso deleted the usage-update branch November 22, 2022 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

imp: adds helpers to get metadata/artifacts from URLs in usage examples #658

imp: adds helpers to get metadata/artifacts from URLs in usage examples #658

gregcaporaso commented Oct 24, 2022 •

edited

Loading

gregcaporaso commented Oct 24, 2022 •

edited

Loading

lizgehret left a comment

lizgehret Oct 25, 2022

lizgehret Oct 25, 2022

lizgehret Oct 25, 2022

lizgehret Oct 25, 2022

lizgehret Oct 25, 2022

gregcaporaso Oct 25, 2022

ebolyen Oct 25, 2022

lizgehret left a comment

ebolyen commented Oct 25, 2022 •

edited

Loading

gregcaporaso commented Oct 25, 2022

ebolyen commented Oct 25, 2022

gregcaporaso commented Oct 26, 2022 •

edited

Loading

gregcaporaso Oct 26, 2022

ebolyen Oct 26, 2022

lizgehret Oct 26, 2022

gregcaporaso Oct 27, 2022

gregcaporaso commented Oct 28, 2022

lizgehret commented Nov 1, 2022

lizgehret commented Nov 17, 2022

lizgehret commented Nov 17, 2022 •

edited

Loading

Oddant1 commented Nov 17, 2022

ebolyen left a comment


		return self.init_artifact(name, factory)

		def init_metadata_from_url(self, name: str, url: str, column: str = None,

	>>> url = f'https://data.qiime2.org/{qiime2.__release__}/tutorials/moving-pictures/sample_metadata.tsv' # noqa: E501
	>>> url = (f'https://data.qiime2.org/{qiime2.__release__}/tutorials/'
	... 'moving-pictures/sample_metadata.tsv')

imp: adds helpers to get metadata/artifacts from URLs in usage examples #658

imp: adds helpers to get metadata/artifacts from URLs in usage examples #658

Conversation

gregcaporaso commented Oct 24, 2022 • edited Loading

gregcaporaso commented Oct 24, 2022 • edited Loading

lizgehret left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizgehret left a comment

Choose a reason for hiding this comment

ebolyen commented Oct 25, 2022 • edited Loading

gregcaporaso commented Oct 25, 2022

ebolyen commented Oct 25, 2022

gregcaporaso commented Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregcaporaso commented Oct 28, 2022

lizgehret commented Nov 1, 2022

lizgehret commented Nov 17, 2022

lizgehret commented Nov 17, 2022 • edited Loading

Oddant1 commented Nov 17, 2022

ebolyen left a comment

Choose a reason for hiding this comment

gregcaporaso commented Oct 24, 2022 •

edited

Loading

gregcaporaso commented Oct 24, 2022 •

edited

Loading

ebolyen commented Oct 25, 2022 •

edited

Loading

gregcaporaso commented Oct 26, 2022 •

edited

Loading

lizgehret commented Nov 17, 2022 •

edited

Loading