Return S3 data links by default when in region #318

jrbourbeau · 2023-10-13T19:29:04Z

I went to run the code referenced here #316 (comment) and got an error because it turns out granule.data_links(access="direct", in_region=True) returns a HTTPS link. This PR fixes that specific case and also makes it so granule.data_links(in_region=True) returns S3 links by default (which seems like it's the expected behavior, but @betolink @MattF-NSIDC let me know if you think otherwise).

I'm testing this PR out now to make sure if in fact fixes things

github-actions · 2023-10-13T19:29:20Z

👈 Launch a binder notebook on this branch for commit 509e42c

I will automatically update this comment whenever this PR is modified

👈 Launch a binder notebook on this branch for commit eee3f3d

👈 Launch a binder notebook on this branch for commit 4db15a7

👈 Launch a binder notebook on this branch for commit 3bcb243

jrbourbeau · 2023-10-13T19:34:49Z

Okay, this seems to working as expected. For reference, here's the testing code I'm using

import os
import tempfile

import coiled
import earthaccess
import xarray as xr

earthaccess.login()
granules = earthaccess.search_data(
    short_name="SEA_SURFACE_HEIGHT_ALT_GRIDS_L4_2SATS_5DAY_6THDEG_V_JPL2205",
    temporal=("2020", "2022"),
    count=2,
)

# Processing function for each data file
@coiled.function(
    region="us-west-2",
    environ={"EARTHDATA_USERNAME": os.environ["EARTHDATA_USERNAME"], "EARTHDATA_PASSWORD": os.environ["EARTHDATA_PASSWORD"]},
    keepalive="20 minutes",
)
def process(granule):
    results = []
    earthaccess.login()
    with tempfile.TemporaryDirectory() as tmpdir:
        files = earthaccess.download([granule], tmpdir)
        for file in files:
            ds = xr.open_dataset(file)
            ds = ds.sel(Latitude=slice(23, 50), Longitude=slice(270, 330))
            ds = ds.SLA.where((ds.SLA >= 0) & (ds.SLA < 10))
            results.append(ds)
    return xr.concat(results, dim="Time")


# Run processing on all the data granules
chunks = process.map(granules)

# Combine and plot results
ds = xr.concat(chunks, dim="Time")
ds.std("Time").plot(figsize=(14, 6), x="Longitude", y="Latitude").figure.savefig("foo.png")

jrbourbeau · 2023-10-16T16:25:40Z

tests/unit/test_results.py

+    assert g.data_links(access="direct")[0].startswith("s3://")
+    assert g.data_links(access="external")[0].startswith("https://")
+    # `in_region` specified
+    assert g.data_links(in_region=True)[0].startswith("s3://")
+    assert g.data_links(in_region=False)[0].startswith("https://")
+    # When `access` and `in_region` are both specified, `access` takes priority
+    assert g.data_links(access="direct", in_region=True)[0].startswith("s3://")
+    assert g.data_links(access="direct", in_region=False)[0].startswith("s3://")
+    assert g.data_links(access="external", in_region=True)[0].startswith("https://")
+    assert g.data_links(access="external", in_region=False)[0].startswith("https://")


I think this is the intended behavior we want, but let me know if I'm missing something.

As a side note, I'm not sure why we have separate access and in_region kwargs for determining if we want to use s3 or https urls. Is one kwarg sufficient?

As a side note, I'm not sure why we have separate access and in_region kwargs for determining if we want to use s3 or https urls. Is one kwarg sufficient?

I can't answer the question directly, but these keywords also feel unintuitive to me. What about access="s3"? To me, "direct" and "external" don't mean anything without more context, but "s3" and "https" do.

I agree that two kwargs is clunky and direct/external aren't the most descriptive names, and we could likely handle it with a single kwarg.

that said, since they are the current interface, we should probably open an issue for possibly refactoring it and not block this PR.

…-links-fixup

jrbourbeau · 2023-10-17T19:48:13Z

earthaccess/results.py

@@ -325,7 +325,6 @@ def data_links(
            else:
                # we are not in us-west-2, even cloud collections have HTTPS links
                return https_links
-        return https_links


This is just cosmetic (this line would never be called, so I decided to remove it)

👍 I tend to go the other way and drop the else statement, but six-of-one...

jrbourbeau · 2023-10-20T20:21:59Z

@betolink @MattF-NSIDC do either of you have bandwidth to take a quick look at this PR? Totally fine if not -- I'm also okay just merging this (I think the actual changes here should be pretty uncontroversial) and owning any follow-up work if there is any.

mfisher87 · 2023-10-20T23:45:28Z

Hey James, I've been refraining from speaking on your questions about intended behavior because I don't know :)

The change itself looks great, I love the addition of a unit test for this aspect of the interface. I'm 100% on board with merging and dealing with any unexpected results as they come! 🚀

jrbourbeau · 2023-10-23T15:14:48Z

Sounds good thanks @mfisher87

It turns out that while I can approve / merge PRs, I don't have sufficient permissions to override the "Review required" check on GitHub. Would you (or someone else) mind approving?

jhkennedy · 2023-10-23T15:29:32Z

earthaccess/results.py

@@ -325,7 +325,6 @@ def data_links(
            else:
                # we are not in us-west-2, even cloud collections have HTTPS links
                return https_links
-        return https_links


👍 I tend to go the other way and drop the else statement, but six-of-one...

jhkennedy · 2023-10-23T15:29:46Z

tests/unit/test_results.py

+    assert g.data_links(access="direct")[0].startswith("s3://")
+    assert g.data_links(access="external")[0].startswith("https://")
+    # `in_region` specified
+    assert g.data_links(in_region=True)[0].startswith("s3://")
+    assert g.data_links(in_region=False)[0].startswith("https://")
+    # When `access` and `in_region` are both specified, `access` takes priority
+    assert g.data_links(access="direct", in_region=True)[0].startswith("s3://")
+    assert g.data_links(access="direct", in_region=False)[0].startswith("s3://")
+    assert g.data_links(access="external", in_region=True)[0].startswith("https://")
+    assert g.data_links(access="external", in_region=False)[0].startswith("https://")


I agree that two kwargs is clunky and direct/external aren't the most descriptive names, and we could likely handle it with a single kwarg.

that said, since they are the current interface, we should probably open an issue for possibly refactoring it and not block this PR.

jrbourbeau · 2023-10-23T15:32:58Z

Thanks @mfisher87 @jhkennedy!

jrbourbeau · 2023-10-23T15:37:41Z

@jhkennedy see #327 for the follow up issue on access / in_region kwargs

mfisher87 · 2023-10-23T15:42:06Z

It turns out that while I can approve / merge PRs, I don't have sufficient permissions to override the "Review required" check on GitHub.

Shoot, let's fix that! My work laptop is put away, and I similarly don't have those permissions on this account, and also can't change branch protection rules (I'll do it on Wednesday if not resolved sooner).

We could make James and other trusted maintainers admins of the repo (I think this is the best way forward so maintainers can also maintain repo settings), or we could remove the branch protection rule or add James to the list of people that can bypass it. @betolink @jrbourbeau what do you think?

MattF-NSIDC · 2023-10-25T16:02:08Z

@jrbourbeau you can bypass the protection rule now if you feel it's needed!

Return S3 data links by default when in region

509e42c

Add unit tests

eee3f3d

jrbourbeau commented Oct 16, 2023

View reviewed changes

jrbourbeau added 2 commits October 16, 2023 11:41

Merge branch 'main' of https://github.com/nsidc/earthaccess into data…

4db15a7

…-links-fixup

Merge branch 'main' of https://github.com/nsidc/earthaccess into data…

3bcb243

…-links-fixup

jrbourbeau commented Oct 17, 2023

View reviewed changes

jrbourbeau mentioned this pull request Oct 20, 2023

Release? #322

Closed

jhkennedy approved these changes Oct 23, 2023

View reviewed changes

jrbourbeau merged commit cfc61a9 into nsidc:main Oct 23, 2023

jrbourbeau deleted the data-links-fixup branch October 23, 2023 15:33

jrbourbeau mentioned this pull request Oct 23, 2023

Consolidate access= and in_region= kwargs? #327

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return S3 data links by default when in region #318

Return S3 data links by default when in region #318

jrbourbeau commented Oct 13, 2023

github-actions bot commented Oct 13, 2023 •

edited

Loading

jrbourbeau commented Oct 13, 2023 •

edited

Loading

jrbourbeau Oct 16, 2023

mfisher87 Oct 20, 2023

jhkennedy Oct 23, 2023

jrbourbeau Oct 17, 2023

jhkennedy Oct 23, 2023

jrbourbeau commented Oct 20, 2023

mfisher87 commented Oct 20, 2023 •

edited

Loading

jrbourbeau commented Oct 23, 2023

jhkennedy Oct 23, 2023

jhkennedy Oct 23, 2023

jrbourbeau commented Oct 23, 2023

jrbourbeau commented Oct 23, 2023

mfisher87 commented Oct 23, 2023

MattF-NSIDC commented Oct 25, 2023

Return S3 data links by default when in region #318

Return S3 data links by default when in region #318

Conversation

jrbourbeau commented Oct 13, 2023

github-actions bot commented Oct 13, 2023 • edited Loading

jrbourbeau commented Oct 13, 2023 • edited Loading

jrbourbeau Oct 16, 2023

Choose a reason for hiding this comment

mfisher87 Oct 20, 2023

Choose a reason for hiding this comment

jhkennedy Oct 23, 2023

Choose a reason for hiding this comment

jrbourbeau Oct 17, 2023

Choose a reason for hiding this comment

jhkennedy Oct 23, 2023

Choose a reason for hiding this comment

jrbourbeau commented Oct 20, 2023

mfisher87 commented Oct 20, 2023 • edited Loading

jrbourbeau commented Oct 23, 2023

jhkennedy Oct 23, 2023

Choose a reason for hiding this comment

jhkennedy Oct 23, 2023

Choose a reason for hiding this comment

jrbourbeau commented Oct 23, 2023

jrbourbeau commented Oct 23, 2023

mfisher87 commented Oct 23, 2023

MattF-NSIDC commented Oct 25, 2023

github-actions bot commented Oct 13, 2023 •

edited

Loading

jrbourbeau commented Oct 13, 2023 •

edited

Loading

mfisher87 commented Oct 20, 2023 •

edited

Loading