Replace pkg_resources with importlib_resources #497

chuckwondo · 2024-03-19T13:33:20Z

Fix "DeprecationWarning: pkg_resources is deprecated as an API." seen when running tests.

In addition:

increase test coverage for formatters.py
replace deprecated set-output in test.yml workflow
remove redundant lines in Makefile
make minor grammatical and formatting fixes to CONTRIBUTING.md
remove old, redundant "Code of Conduct" section from CONTRIBUTING.md and insert link to CODE_OF_CONDUCT.md
fix bare links in CODE_OF_CONDUCT.md

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

mfisher87 · 2024-03-19T15:10:32Z

These doc fixes are so helpful, thank you for including them!

Makefile

earthaccess/formatters.py

mfisher87 · 2024-03-19T16:38:23Z

Woops, didn't notice workflows waiting for approval. 🐌 🚀

mfisher87 · 2024-03-19T16:58:18Z

@chuckwondo we're just about to start a hack day, hope you'll be joining us again :)

danielfromearth · 2024-03-19T17:25:11Z

Just linking this to the other PR that this supersedes: #452, which I think should be closed after this one is merged.

mfisher87 · 2024-03-19T20:32:31Z

I tried putting Closes #452 in the OP but that didn't seem to work the way I wanted it to. I removed it. Can we close #452 now so we don't have to remember?

chuckwondo · 2024-03-19T20:40:36Z

@mfisher87, I made a change that should fix the broken python 3.8 build. It also happens to simplify things a bit.

chuckwondo · 2024-03-19T20:42:44Z

Just linking this to the other PR that this supersedes: #452, which I think should be closed after this one is merged.

@danielfromearth, thanks for making note of this. It totally slipped my mind that I had previously commented on your issue there! This should close the loop on that.

mfisher87 · 2024-03-19T20:51:39Z

And thank you @danielfromearth for your work on this problem!

chuckwondo · 2024-03-19T21:04:35Z

Ugh! Updated ci/environment-mindeps.yaml to include importlib-resources, which I had added to pyproject.toml. This seems like an accident waiting to happen (it just did). Any idea why the redundancy that forces us to keep these files in sync?

jhkennedy · 2024-03-19T21:27:05Z

Ugh! Updated ci/environment-mindeps.yaml to include importlib-resources, which I had added to pyproject.toml. This seems like an accident waiting to happen (it just did). Any idea why the redundancy that forces us to keep these files in sync?

Unfortunately, the environment-* YAMLs are conda environments and dependencies, while the deps in pyproject.toml are Poetry ones that get pulled from PyPI. There's no real way to avoid the duplication due to different package repositories and managers.

And really, even if we went to just poetry environments, there's no way to specify "build an environment from the minimum dependencies" in Poetry, but we could with uv and only use that (dropping conda). Something to add to #374

It's good to have conda env test coverage as well though as many of our users will be using conda.

jhkennedy · 2024-03-19T21:29:37Z

pyproject.toml

@@ -44,6 +44,7 @@ multimethod = ">=1.8"
 python-dateutil = ">=2.8.2"
 kerchunk = { version = ">=0.1.2", optional = true }
 dask = { version = ">=2022.1.0", optional = true }
+importlib-resources = "^6.3.2"


Do we need to pin down to the patch version here?

Not necessarily, but I simply used poetry add importlib-resources to add it. There are other deps pinned similarly, but there is certainly no consistency.

This seems to point to a gap in the contributing guide regarding adding dependencies; not only how to add them, but also where to add them (other than pyproject.toml).

I'm happy to add a note in the contributing guide, if you can provide guidance.

Also, happy to adjust the version pin here accordingly, if there's a preference.

Also, a brief explanation of the why (in addition to the how and where) might be good to add to the contributing guide.

Again, happy to add all such details to the contributing guide as part of this PR, if someone can provide a few sentences.

There's been a bit of lively discussion around dependencies in #468, but generally, a restrictive top-pin (down to the patch version) like this will force this package to break quickly for users. So we should remove the upper bound if we don't need it.

Suggested change

importlib-resources = "^6.3.2"

importlib-resources = ">=6.3.2"

Similarly, is there a reason for this version being the lower bound?

This seems to point to a gap in the contributing guide regarding adding dependencies; not only how to add them, but also where to add them (other than pyproject.toml).

Yes, agreed, we should add that. Especially since poetry add adds upper bounds to dependencies with ^, which is bad for libraries (and will not change python-poetry/poetry#3427)

Those steps sound right to me. For (1) I'd be inclined to have the users just manually add it to the pyproject.toml instead of having to tell them to effectively manually do it again in (2), but (1) does update the lockfile too.

For 2 we could tell them to use poetry-relax but that should probably be done in isolation with its own PR later.

Cool. Can you provide the "why" in the 2 spots I mentioned? Just a sentence or 2 in each spot to provide context for contributors.

What's "a few versions behind the latest"? If you're referring to importlib-resources, that version is the latest as of earlier today.

Bah, I thought I looked earlier and latest was 7.0.2; no idea what I looked at 😫

@chuckwondo this is getting large -- perhaps we should move the contributing guide discussion to its own issue and subsequent PR?

As for the version, since we're still a poetry project, let's just go with:

importlib-resources = ">=6.3.2"

for now since that's what it decided and the rest were likely added that way.

As for the why:

On the line added to pyproject.toml by poetry, change ^ to >= (perhaps add a "why" bit here)

because upper bounds can easily result in unsolvable user environments for Python libraries like Earthaccess. For more information, see the poetry-relax README, especially the references.

Add the same dependency to ci/environment-mindeps.yaml. For a required dep, pin the exact version, but for a testing dep, leave unpinned (again, a "why" comment here might also prove helpful)

I don't actually know the reasoning here... looks like it's because we pip install with --no-deps and only use the dependencies in the conda environment, so we need all dependencies reflected here.
https://github.com/nsidc/earthaccess/blob/main/.github/workflows/test-mindeps.yml

@mfisher87 @betolink got a sentence for this?

ci/environment-mindeps.yaml

Fix "DeprecationWarning: pkg_resources is deprecated as an API." seen when running tests. In addition: - increase test coverage for `formatters.py` - replace deprecated `set-output` in `test.yml` workflow - remove redundant lines in `Makefile` - make minor grammatical and formatting fixes to `CONTRIBUTING.md` - remove old, redundant "Code of Conduct" section from `CONTRIBUTING.md` and insert link to `CODE_OF_CONDUCT.md` - fix bare links in `CODE_OF_CONDUCT.md`

chuckwondo · 2024-03-20T23:04:59Z

@jhkennedy and @mfisher87, I believe I addressed the dependency pinning issue. I also added a small "Managing Dependencies" section to the contributing guide. Please let me know if I missed anything.

jhkennedy

looks good to me. @mfisher87 did you want to tak another look?

chuckwondo · 2024-03-26T12:19:47Z

@mfisher87, does this look good?

chuckwondo · 2024-03-28T12:56:21Z

@jhkennedy can you take a look at how/why one of the checks was cancelled and rerun? I see, "Canceling since a higher priority waiting request for 'refs/pull/497/merge' exists," but can't tell exactly what caused it.

jhkennedy · 2024-03-28T17:37:48Z

@jhkennedy can you take a look at how/why one of the checks was cancelled and rerun? I see, "Canceling since a higher priority waiting request for 'refs/pull/497/merge' exists," but can't tell exactly what caused it.

I'm confused by that as well. I believe it's related to the concurrency setting:
https://github.com/nsidc/earthaccess/blob/main/.github/workflows/test-mindeps.yml#L8-L12

But not sure why. I don't have time to dig in right now (it works in some PRs but not others), but I'll open an issue.

For now, I think it's safe to ignore it as the test ran fine in your fork:
https://github.com/chuckwondo/earthaccess/actions/runs/8367002850

mfisher87 · 2024-03-29T00:18:07Z

Hey, so sorry all for missing your mentions! 👀

mfisher87

LGTM! I have lost some context in my delay to respond to this issue and I apologize for that 😿

chuckwondo · 2024-04-01T16:56:23Z

Can somebody merge this PR?

jhkennedy · 2024-04-01T17:46:16Z

@chuckwondo done!

mfisher87 · 2024-04-01T20:40:42Z

Thanks @jhkennedy . @chuckwondo do you want merge permissions? We don't have any expectations of time commitment for maintainers.

chuckwondo · 2024-04-01T21:44:21Z

@mfisher87, thanks for the offer for merge permissions. I don't yet feel comfortable being granted such permissions, but perhaps after a few more PRs and "hack days" I'll take you up on the offer.

mfisher87 · 2024-04-01T23:27:19Z

I'll be sure to ask again later, then! ;)

mfisher87 reviewed Mar 19, 2024

View reviewed changes

Makefile Show resolved Hide resolved

mfisher87 previously approved these changes Mar 19, 2024

View reviewed changes

Makefile Show resolved Hide resolved

earthaccess/formatters.py Show resolved Hide resolved

chuckwondo dismissed mfisher87’s stale review via ad69960 March 19, 2024 20:34

chuckwondo requested a review from mfisher87 March 19, 2024 20:39

danielfromearth mentioned this pull request Mar 19, 2024

replace deprecated pkg_resources with importlib.resources #452

Closed

chuckwondo changed the title ~~Replace pkg_resources with importlib.resources~~ Replace pkg_resources with importlib_resources Mar 19, 2024

chuckwondo mentioned this pull request Mar 19, 2024

DeprecationWarning: pkg_resources is deprecated as an API. #419

Closed

jhkennedy reviewed Mar 19, 2024

View reviewed changes

jhkennedy mentioned this pull request Mar 19, 2024

Switch away from Poetry for build (to setuptools?) #374

Closed

chuckwondo mentioned this pull request Mar 19, 2024

483 refactor search #494

Merged

chuckwondo force-pushed the use-importlib-resources branch from a5bae16 to c72e18e Compare March 20, 2024 22:52

chuckwondo requested a review from jhkennedy March 20, 2024 22:53

chuckwondo force-pushed the use-importlib-resources branch from c72e18e to 741f5e7 Compare March 20, 2024 23:01

jhkennedy approved these changes Mar 22, 2024

View reviewed changes

jhkennedy mentioned this pull request Mar 28, 2024

test-mindeps erroneously cancelling test-mindeps jobs #503

Closed

mfisher87 approved these changes Mar 29, 2024

View reviewed changes

jhkennedy merged commit c140c61 into nsidc:main Apr 1, 2024
10 of 11 checks passed

chuckwondo deleted the use-importlib-resources branch September 17, 2024 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace pkg_resources with importlib_resources #497

Replace pkg_resources with importlib_resources #497

chuckwondo commented Mar 19, 2024 •

edited by mfisher87

Loading

mfisher87 commented Mar 19, 2024

mfisher87 commented Mar 19, 2024

mfisher87 commented Mar 19, 2024

danielfromearth commented Mar 19, 2024

mfisher87 commented Mar 19, 2024

chuckwondo commented Mar 19, 2024

chuckwondo commented Mar 19, 2024

mfisher87 commented Mar 19, 2024

chuckwondo commented Mar 19, 2024

jhkennedy commented Mar 19, 2024

jhkennedy Mar 19, 2024

chuckwondo Mar 19, 2024

chuckwondo Mar 19, 2024

jhkennedy Mar 19, 2024 •

edited

Loading

jhkennedy Mar 19, 2024

chuckwondo Mar 20, 2024

jhkennedy Mar 20, 2024

jhkennedy Mar 20, 2024

jhkennedy Mar 20, 2024

jhkennedy Mar 20, 2024 •

edited

Loading

chuckwondo commented Mar 20, 2024

jhkennedy left a comment

chuckwondo commented Mar 26, 2024

chuckwondo commented Mar 28, 2024

jhkennedy commented Mar 28, 2024

mfisher87 commented Mar 29, 2024

mfisher87 left a comment

chuckwondo commented Apr 1, 2024

jhkennedy commented Apr 1, 2024

mfisher87 commented Apr 1, 2024

chuckwondo commented Apr 1, 2024

mfisher87 commented Apr 1, 2024

	importlib-resources = "^6.3.2"
	importlib-resources = ">=6.3.2"

Replace pkg_resources with importlib_resources #497

Replace pkg_resources with importlib_resources #497

Conversation

chuckwondo commented Mar 19, 2024 • edited by mfisher87 Loading

mfisher87 commented Mar 19, 2024

mfisher87 commented Mar 19, 2024

mfisher87 commented Mar 19, 2024

danielfromearth commented Mar 19, 2024

mfisher87 commented Mar 19, 2024

chuckwondo commented Mar 19, 2024

chuckwondo commented Mar 19, 2024

mfisher87 commented Mar 19, 2024

chuckwondo commented Mar 19, 2024

jhkennedy commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhkennedy Mar 19, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhkennedy Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

chuckwondo commented Mar 20, 2024

jhkennedy left a comment

Choose a reason for hiding this comment

chuckwondo commented Mar 26, 2024

chuckwondo commented Mar 28, 2024

jhkennedy commented Mar 28, 2024

mfisher87 commented Mar 29, 2024

mfisher87 left a comment

Choose a reason for hiding this comment

chuckwondo commented Apr 1, 2024

jhkennedy commented Apr 1, 2024

mfisher87 commented Apr 1, 2024

chuckwondo commented Apr 1, 2024

mfisher87 commented Apr 1, 2024

chuckwondo commented Mar 19, 2024 •

edited by mfisher87

Loading

jhkennedy Mar 19, 2024 •

edited

Loading

jhkennedy Mar 20, 2024 •

edited

Loading