Skip to content

Remove testing of on-prem (ECS) collections - #1075

Merged
betolink merged 10 commits into
mainfrom
byebye-ecs
Sep 16, 2025
Merged

Remove testing of on-prem (ECS) collections#1075
betolink merged 10 commits into
mainfrom
byebye-ecs

Conversation

@jhkennedy

@jhkennedy jhkennedy commented Aug 22, 2025

Copy link
Copy Markdown
Contributor

More and more on-prem (ECS) collections are being migrated to the cloud, and when that happens, the concept ids change, breaking our integration tests. The most popular collections, which we've targeted for testing, are also the collections prioritized for migration, so this will likely be a recurring issue.

Important

This PR is more to prompt discussion, since I find it easier to see what changes we're discussing in this form. Hopefully, we end up with a mergeable PR as well.

When a collection migrates, ideally, we'd update the dataset list and continue testing the cloud version of the collection. But finding the new collection is not necessarily straight-forward, and we've typically just deleted the failing collections from the list (#1036, https://github.com/nsidc/earthaccess/pull/1061/files#diff-30fb83bf5c1cdf245739c1ab6a7a9ae06fdac2df265d380ddcbd35e0c10489e7).

I propose we simply stop testing on-prem collections and instead develop tooling to periodically regenerate the popular collections list, excluding on-prem collections.

However, it's likely that the new cloud version of the collection won't appear popular in metrics for a time, and so my proposal above may miss popular collections during that period. I think that's okay, but we could additionally develop tooling that discovers the new concept id for a migrated collection and opens a PR to update the list.

Or, maybe we should do both? If we decide to do both, it's likely worth removing on-prem collections until we've got the migration-pr tooling developed so that we don't keep having/accepting PRs with failing integration tests.

Updating the list

Pull Request (PR) draft checklist - click to expand
  • Please review our
    contributing documentation
    before getting started.
  • Populate a descriptive title. For example, instead of "Updated README.md", use a
    title such as "Add testing details to the contributor section of the README".
    Example PRs: #763
  • Populate the body of the pull request with:
  • Update CHANGELOG.md with details about your change in a section titled
    ## Unreleased. If such a section does not exist, please create one. Follow
    Common Changelog for your additions.
    Example PRs: #763
  • Update the documentation and/or the README.md with details of changes to the
    earthaccess interface, if any. Consider new environment variables, function names,
    decorators, etc.

Click the "Ready for review" button at the bottom of the "Conversation" tab in GitHub
once these requirements are fulfilled. Don't worry if you see any test failures in
GitHub at this point!

Pull Request (PR) merge checklist - click to expand

Please do your best to complete these requirements! If you need help with any of these
requirements, you can ping the @nsidc/earthaccess-support team in a comment and we
will help you out!

  • Add unit tests for any new features.
  • Apply formatting and linting autofixes. You can add a GitHub comment in this Pull
    Request containing "pre-commit.ci autofix" to automate this.
  • Ensure all automated PR checks (seen at the bottom of the "conversation" tab) pass.
  • Get at least one approving review.

📚 Documentation preview 📚: https://earthaccess--1075.org.readthedocs.build/en/1075/

@jhkennedy
jhkennedy requested a review from mfisher87 August 22, 2025 18:11
@github-actions

github-actions Bot commented Aug 22, 2025

Copy link
Copy Markdown

Binder 👈 Launch a binder notebook on this branch for commit e9063c5

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit 37771a9

Binder 👈 Launch a binder notebook on this branch for commit b306ab8

Binder 👈 Launch a binder notebook on this branch for commit 646eb81

Binder 👈 Launch a binder notebook on this branch for commit c337947

Binder 👈 Launch a binder notebook on this branch for commit 9898c07

Binder 👈 Launch a binder notebook on this branch for commit 94f24ac

Binder 👈 Launch a binder notebook on this branch for commit 1aa3f97

@jhkennedy

jhkennedy commented Aug 22, 2025

Copy link
Copy Markdown
Contributor Author

It's worth looking through where else we point to ECS collections:

  1. [removed] notebooks/Demo.ipynb and notebooks/EarthdataLogin.ipynb uses some NSIDC_ECS collections
  2. we use ECS collections in our docstring for earthaccess.search.concept_id
  3. earthaccess.daac has a list of on-prem providers -- eventually (supposedly) we'll be able to remove them because there won't be any on-prem collections anymore.
  4. [removed] tests/integration/test_onprem_download.py and tests/integration/test_onprem_open.py -- do we want to keep testing this explicitly?
  5. test/unit/fixtures has a pile of on prem data cached from our testing

I think (1), (2), and (5) are worth switching to cloud collections now, but I'm not sure what our longer-term plans with (3) and (4) are.

@betolink

Copy link
Copy Markdown
Member

I think we should remove the notebooks in the folder, they are outdated examples anyways and the documentation has all we need to demonstrate the core functionality now. What do you guys think? @andypbarrett @asteiker @mfisher87

@asteiker

Copy link
Copy Markdown
Contributor

@betolink @jhkennedy Yes I agree let's remove the /notebooks folder. I say this after only quickly skimming the content but it does look out of date and we should only be focusing on supporting RTD content in /docs anyway.

Agreed on removing NSIDC_ECS from (2). As far as the listing in https://github.com/nsidc/earthaccess/blob/main/earthaccess/daac.py (is this the same as earthaccess.daac? ) I agree we can remove all of these eventually but every DAAC is on a different timeline. I can only speak for NSIDC: We won't have any collections publicly accessible in our ECS provider after Jan 1 2026 so I can at least coordinate the removal of those references once we've fully hidden our data.

For (4), again it may depend on what others prefer based on the different DAAC migration timelines. Personally I don't think we need to keep testing NSIDC_ECS data at this point.

@jhkennedy

Copy link
Copy Markdown
Contributor Author

Alright, removed the notebooks/ folder!

@mfisher87

Copy link
Copy Markdown
Member

I propose we simply stop testing on-prem collections and instead develop tooling to periodically regenerate the popular collections list, excluding on-prem collections.

For example, a bot which opens a monthly PR to update the lists?

@betolink

betolink commented Sep 5, 2025

Copy link
Copy Markdown
Member

@jhkennedy what's missing on this one?

@jhkennedy

Copy link
Copy Markdown
Contributor Author

Mostly just figuring out why the integration tests aren't passing, but potentially also the remaining items in this list:
#1075 (comment)

@mfisher87

Copy link
Copy Markdown
Member

@betolink

Copy link
Copy Markdown
Member

status() tests and GES_DISC related tests are failing, GES_DISC seems like the token auth is no longer working for some reason

curl -Lv \
  -H "Authorization: Bearer $EDL_TOKEN" \
  -H "Range: bytes=0-16383" \
  "https://data.gesdisc.earthdata.nasa.gov/data/MERRA2/M2T1NXSLV.5.12.4/2024/01/MERRA2_400.tavg1_2d_slv_Nx.20240101.nc4.dmrpp"

😿

@jhkennedy

Copy link
Copy Markdown
Contributor Author

status() tests and GES_DISC related tests are failing, GES_DISC seems like the token auth is no longer working for some reason

curl -Lv \
  -H "Authorization: Bearer $EDL_TOKEN" \
  -H "Range: bytes=0-16383" \
  "https://data.gesdisc.earthdata.nasa.gov/data/MERRA2/M2T1NXSLV.5.12.4/2024/01/MERRA2_400.tavg1_2d_slv_Nx.20240101.nc4.dmrpp"

😿

might be widespread; bearer tokens aren't working to get s3 credentials right now from ASF's cumulus and sentinel1 endpoints.

@betolink

betolink commented Sep 11, 2025

Copy link
Copy Markdown
Member

@jhkennedy I think we should remove all ECS/on-prem tests and merge this tomorrow if possible.

@jhkennedy

Copy link
Copy Markdown
Contributor Author

Works for me! I'm going to be mostly out tomorrow and probably busy today, so feel free to Sheppard it along from here w/out me.

@jhkennedy
jhkennedy marked this pull request as ready for review September 11, 2025 18:56

@betolink betolink left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this bye bye ECS. Merging soon

@betolink
betolink merged commit 3c5bd3f into main Sep 16, 2025
10 of 14 checks passed
@betolink
betolink deleted the byebye-ecs branch September 16, 2025 01:27
@github-project-automation github-project-automation Bot moved this to ✅ Done in earthaccess Mar 3, 2026
@mfisher87 mfisher87 removed this from earthaccess Mar 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants