Speed up `mast.Observations.get_cloud_uris()` #2145

barentsen · 2021-09-08T01:19:01Z

When using astroquery.mast, it is currently very slow to download multiple data products in cloud-enabled mode.

The main reason for the poor performance is that the Observations.get_cloud_uris() helper method is very slow at retrieving AWS S3 URIs. For example, the following example requires >2 minutes to obtain URIs for 186 data products:

This is unfortunate because slow data access makes the cloud-enabled mode less attractive.

The main reason Observations.get_cloud_uris() is slow is because every data products triggers a separate HTTP request to MAST's path_lookup API via the astroquery.mast.utils.mast_relative_path() helper function.

This PR proposes to modify these functions such that MAST's path_lookup API is called in chunks of 50 products at once, allowing Observations.get_cloud_uris() to execute much faster. For example, the demo above is ~7x faster when using this PR branch:

(Note: the remaining 20s used by get_cloud_uris() is spent polling every cloud URI to verify that it exists before returning it to the user, because not every MAST product is mirrored in the cloud. This will be harder to speed up, but fortunately this may be fast when running in the cloud.)

codecov · 2021-09-08T01:25:56Z

Codecov Report

Attention: Patch coverage is 2.27273% with 43 lines in your changes missing coverage. Please review.

Project coverage is 66.44%. Comparing base (aff2ac5) to head (4c53a19).
Report is 839 commits behind head on main.

Files with missing lines	Patch %	Lines
astroquery/mast/cloud.py	0.00%	26 Missing ⚠️
astroquery/mast/utils.py	5.55%	17 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2145      +/-   ##
==========================================
+ Coverage   66.10%   66.44%   +0.34%     
==========================================
  Files         234      235       +1     
  Lines       18065    18123      +58     
==========================================
+ Hits        11941    12042     +101     
+ Misses       6124     6081      -43

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bsipocz · 2022-02-24T04:12:15Z

This now needs and rebase. Also, @jaymedina could you assign it to the person who are supposed to review this, or cc them on the PR? Thanks!

jaymedina · 2022-02-24T19:56:23Z

Hi @bsipocz I'll review this in conjunction with @tomdonaldson (I think he started a review already?)

Edit:
I've started a review for this PR @barentsen. This looks really great, and the improvement in time is fantastic. This will need a rebase and re-testing before anything else tho, because I'm pretty sure get_cloud_uri_list will break for GALEX data retrieval from the cloud. GALEX data retrieval requires a minor tweak in the output of mast_relative_path because of the location it's been uploaded onto S3. I've made a review comment below that goes into more detail.

jaymedina

Looks solid, the main suggestion I have is what I mentioned about the GALEX relative path tweak that I discovered was necessary for GALEX on S3. Also added some minor comments, but in general this looks good.

I'd say the first thing to do right now is rebase and rerun unit tests, to verify that GALEX indeed breaks without a tweak to the mast_relative_path output.

jaymedina · 2022-03-02T17:10:16Z

astroquery/mast/cloud.py


-        return [self.get_cloud_uri(product, include_bucket, full_url) for product in data_products]
+        paths = utils.mast_relative_path(data_products["dataURI"])


This will break for GALEX data retrieval from the cloud, unfortunately. See here for how I tweaked this output in a recent PR following the new GALEX availability on the cloud:

astroquery/astroquery/mast/cloud.py

Line 119 in 2b3d954

if 'galex' in path:

Your two options would be to either:

Make a tweak similar to this in get_cloud_uri_list

Make the tweak in mast_relative_path - this is probably the cleanest option because then we can remove my tweak from cloud.py.

re this suggestion: in the cloud WG we're working on generalizing these methods and move them out from mast. So a tweak at the place of usage, or a generically usable kwarg would be preferable rather than a hard-wired conditional on mast specific strings.

What I did was move the path.strip() logic out of cloud.py and into util.py's mast_relative_path function. In other words, I essentially went with option 2. listed above. And because this is a MAST-specific function in utils, it makes sense for MAST-specific tweaks to take place in there. Let me know if that works for you!

astroquery/mast/cloud.py

bsipocz · 2022-09-08T04:02:25Z

@barentsen @jaymedina - what are the plans with this PR?

bsipocz · 2022-11-09T06:11:59Z

Ping @barentsen @jaymedina @tomdonaldson, do you have any plans for finishing up this PR? I expect it's reasonably close for being ready.

bsipocz · 2023-07-06T23:04:27Z

ping @jaymedina!

jaymedina · 2023-07-07T16:34:59Z

I'll take a look at this and make updates accordingly.

… using a single API request

…s. Added change log entry.

…tes.

jaymedina · 2023-07-11T23:21:19Z

I've rebased, adapted the code to account for GALEX cloud products, updated error handling and unit tests, moved the MAST-specific string manipulation out of cloud.py, and squashed the commits into 4. I think this is ready for another review @bsipocz , thanks in advance.

bsipocz

Thanks for picking this up and pushing it through the finish line.

I have some minor comments, mostly asking for clarifications.

bsipocz · 2023-07-25T23:26:34Z

astroquery/mast/cloud.py

-            path = path.lstrip("/mast/")
+        # Making sure we got at least 1 URI from the query above.
+        if len(uri_list) == 0:
+            raise InvalidQueryError("No products found on the cloud for this query.")


Is this an InvalidQueryError or rather a NoResultsWarning? We haven't previously raised the error, but if you think the query has an error in it, rather than just there is no result, then we can raise it from now on (probably would require its own changelog entry)

I chose to raise an error rather than a warning so that the output error message is more helpful. Here is what it looks like when I use a NoResultsWarning:

The final TypeError is an indirect result of the empty uri list, so it's a vague error message.

Raising a custom InvalidQueryError makes things more clear:

Maybe then a conditional is needed to return the empty result early, before going into the Typeerror. The point is basically to use the InvalidqueryError when there is an issue with the user input. Not having data, imo is not in this category.

On second thought, a user will realize that their call to Observations.get_cloud_uri(product) returns an empty list before calling Observations.download_file, since they have to feed the uri in string form and by default the uri gets returned as a list even when it's 1 URI. So this error handling thing might be redundant.

bsipocz · 2023-07-25T23:27:28Z

astroquery/mast/cloud.py

-
-        warnings.warn("Unable to locate file {}.".format(data_product['productFilename']), NoResultsWarning)
-        return None
+            return uri_list[0]


Why only the first element, what is expected to be included in the rest?

The output for get_cloud_uri_list is always a list, even when the output returns only 1 URI, so the sole element gets indexed out before being returned. I just committed some commentary above this line for clarity.

It's indeed a good way to handle the one element lists. I wonder what happens when there are more than one, would the rest just be ignored?

There wouldn't be a case where a request for a single cloud URI would return more than one because of L171 in util.py of this branch, but I can raise an AssertionError here to ensure that there is only 1 element in the list, what do you think?

I'm really going into bikeshedding now, but if that's the case, why does it have to be a list to begin with?

Also, L173 is never reached?

Maybe just a comment about this would be enough, no need for the assertion.

Sorry, just revisiting this branch in a while, what assertion are you referring to? I added a comment above L118 in this branch explaining why the zero-indexing has to happen. It's because uri_list is declared as a list in get_cloud_uri_list which get_cloud_uri wraps around. Everything is wrapped around get_cloud_uri_list because that method calls utils.mast_relative_path which has the new chunking functionality that Geert implemented, which speeds up the call by making 1 request for multiple URIs rather than 1 request per URI.

bsipocz · 2023-07-25T23:31:22Z

astroquery/mast/cloud.py

+                    # Use `head_object` to verify that the product is available on S3 (not all products are)
+                    s3_client.head_object(Bucket=self.pubdata_bucket, Key=path)
+                    if include_bucket:
+                        path = "s3://{}/{}".format(self.pubdata_bucket, path)


I don't necessarily like the overwriting of variables here as when this has to be debugged it's a bit more difficult to distinguish between the path from the iteration vs the path here. Maybe use s3_path?

bsipocz · 2023-07-25T23:37:51Z

astroquery/mast/utils.py

+                                   {"uri": chunk})
+        json_response = response.json()
+        for uri in chunk:
+            path = json_response.get(uri[1])["path"]


I don't see what's expected to be in uri here, could you explain it a bit, so I see why the second element?

bsipocz · 2023-07-25T23:46:12Z

astroquery/mast/tests/test_mast_remote.py

@@ -405,28 +405,32 @@ def test_get_cloud_uri(self):

    def test_get_cloud_uris(self):
        pytest.importorskip("boto3")
-        test_obs_id = '25568122'
+        test_obs_ids = ["25568122", "31411"]


do this with pytest.mark.parametrize instead of an internal loop

pep8speaks · 2023-11-09T11:31:11Z

Hello @barentsen! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file astroquery/mast/cloud.py:

Line 115:24: E711 comparison to None should be 'if cond is None:'

bsipocz · 2023-11-09T11:31:30Z

Close/reopen to retrigger CI (as logs were long gone now)

bsipocz · 2023-11-09T14:09:03Z

Needs a rebase for RTD fix and some PEP8 fixes before final review.

jaymedina · 2023-11-28T23:45:34Z

Hi @bsipocz and @keflavich, I'm tagging the interim astroquery.mast maintainer, Julie Imig (@astrojimig). She will be taking over remaining PRs and will be the liaison for MAST-related issues after I'm gone on December 1st.

bsipocz · 2023-12-08T17:58:23Z

@astrojimig - I suppose the conflict is coming from the changes from your recently merged panstarss branch. Anyway, it would be nice to finally wrap this PR up and merge it.

Could you try and rebase this one? I'm not sure you inherited the access to Geert's branch from Jenny.

astrojimig · 2023-12-08T21:40:32Z

@bsipocz Sure, I'll try to work on this next week!

snbianco · 2024-07-12T14:03:46Z

@bsipocz Working on this now! This might just be my own misunderstanding of GitHub, but do I need some sort of permissions to push to this branch? Whenever I try, I get a permission denied error. Is there an alternate method I should be using?

bsipocz · 2024-07-12T16:55:59Z

@bsipocz Working on this now! This might just be my own misunderstanding of GitHub, but do I need some sort of permissions to push to this branch? Whenever I try, I get a permission denied error. Is there an alternate method I should be using?

You cannot push to someone else's fork by default, so you will need to take the applicable changes from here. I have reviewed this a couple of times, but things also have changed in the module, so I'm not certain which way is more efficient and useful (trying to keep these as is and rebase and fix from there, or look into the changes and apply the ones that are still relevant to your other PR (and then just rename your method and add fixes to it so old usage of get_cloud_uris() will work with it, too)

snbianco · 2024-07-16T13:05:29Z

Changes integrated into #3064

barentsen changed the title ~~Speed up mast.Observations.get_cloud_uris by obtaining multiple URIs at once~~ Speed up mast.Observations.get_cloud_uris() Sep 8, 2021

bsipocz requested a review from ceb8 September 8, 2021 01:26

bsipocz added the mast label Sep 8, 2021

bsipocz requested a review from tomdonaldson November 13, 2021 18:53

bsipocz removed the request for review from ceb8 February 24, 2022 04:12

jaymedina assigned barentsen Feb 24, 2022

jaymedina self-requested a review February 24, 2022 19:56

jaymedina suggested changes Mar 2, 2022

View reviewed changes

bsipocz added this to the v0.4.7 milestone Jul 26, 2022

Speed up mast.Observations.get_cloud_uris by obtaining multiple paths…

8710d25

… using a single API request

jaymedina force-pushed the speedup-get-cloud-uris branch from b42de94 to 94d9769 Compare July 8, 2023 02:11

barentsen and others added 2 commits July 11, 2023 19:18

Support large numbers of products by splitting the request into chunk…

39d7e4e

…s. Added change log entry.

Adapting changes to support GALEX cloud products. Error handling upda…

8d87d84

…tes.

jaymedina force-pushed the speedup-get-cloud-uris branch from 8438d9e to 1ce2697 Compare July 11, 2023 23:19

jaymedina force-pushed the speedup-get-cloud-uris branch from 6916a1b to cfdfcbc Compare July 11, 2023 23:29

Improved test_get_cloud_uris to test for a GALEX observation

0a30aa2

jaymedina force-pushed the speedup-get-cloud-uris branch from f181c56 to 0a30aa2 Compare July 12, 2023 03:54

bsipocz reviewed Jul 25, 2023

View reviewed changes

jaymedina added 4 commits July 26, 2023 10:19

Parametrization of unit test

e4cf69a

Renaming path to s3_path

42a4642

codestyle

493e467

Adding commentary to chunk iteration

b865ea0

jaymedina added 3 commits July 26, 2023 10:59

Adding commentary to uri_list indexing for only 1 URI

f1e5852

New conditional to catch no results

1e9750b

Updating NoResultsWarning

4c53a19

bsipocz closed this Nov 9, 2023

bsipocz reopened this Nov 9, 2023

bsipocz removed this from the v0.4.7 milestone Feb 17, 2024

bsipocz mentioned this pull request Jul 11, 2024

Streamlined method to get list of cloud URIs #3064

Merged

snbianco closed this Jul 16, 2024


		return [self.get_cloud_uri(product, include_bucket, full_url) for product in data_products]
		paths = utils.mast_relative_path(data_products["dataURI"])

Speed up mast.Observations.get_cloud_uris() #2145

Speed up mast.Observations.get_cloud_uris() #2145

Conversation

barentsen commented Sep 8, 2021 • edited Loading

codecov bot commented Sep 8, 2021 • edited Loading

Codecov Report

bsipocz commented Feb 24, 2022

jaymedina commented Feb 24, 2022 • edited Loading

jaymedina left a comment

Choose a reason for hiding this comment

jaymedina Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymedina Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

bsipocz commented Sep 8, 2022

bsipocz commented Nov 9, 2022

bsipocz commented Jul 6, 2023

jaymedina commented Jul 7, 2023

jaymedina commented Jul 11, 2023

bsipocz 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

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

pep8speaks commented Nov 9, 2023

bsipocz commented Nov 9, 2023

bsipocz commented Nov 9, 2023

jaymedina commented Nov 28, 2023 • edited Loading

bsipocz commented Dec 8, 2023

astrojimig commented Dec 8, 2023

snbianco commented Jul 12, 2024

bsipocz commented Jul 12, 2024

snbianco commented Jul 16, 2024

Speed up `mast.Observations.get_cloud_uris()` #2145

Speed up `mast.Observations.get_cloud_uris()` #2145

barentsen commented Sep 8, 2021 •

edited

Loading

codecov bot commented Sep 8, 2021 •

edited

Loading

jaymedina commented Feb 24, 2022 •

edited

Loading

jaymedina Mar 2, 2022 •

edited

Loading

jaymedina Jul 11, 2023 •

edited

Loading

jaymedina commented Nov 28, 2023 •

edited

Loading