Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use Importlib.resources instead of (deprecated and slower) pkg resources.resource_filename #2157

Merged
merged 6 commits into from
Jan 5, 2024

Conversation

moustakas
Copy link
Member

This is a cleanup PR which should fix #2018. It touches a lot of files but the changes are relatively minimal.

Note that a few modules imported pkg_resources.resource_filename but didn't actually use it, so I removed that import entirely. Also, I replaced pkg_resources.resource_exists() with importlib.resources.files('desispec').joinpath('desired_file_in_desispec').is_file(), as recommended here.

To find the impacted files, I did:

cd /path/to/desispec
grep -r --include="*.py" 'pkg_resources' .

and then I also checked all the executable scripts in desispec/bin/.

Hopefully I'm not missing any files but those can be cleaned up as needed in future PRs; this PR gets most of them (for desispec at least).

Finally, I ran pytest at NERSC and I think everything passed, but I'm not sure how to interpret "2 xfailed" in the log, below (if this is something I introduced or if it's a known issue):

% pytest
[snip]
============================================================================= warnings summary ==============================================================================
py/desispec/test/test_binscripts.py::TestBinScripts::test_compute_sky
py/desispec/test/test_sky.py::TestSky::test_subtract_sky
py/desispec/test/test_sky.py::TestSky::test_uniform_resolution
  /global/common/software/desi/perlmutter/desiconda/20230111-2.1.0/conda/lib/python3.10/site-packages/numpy/core/fromnumeric.py:3474: RuntimeWarning: Mean of empty slice.
    return _methods._mean(a, axis=axis, dtype=dtype,

py/desispec/test/test_binscripts.py::TestBinScripts::test_compute_sky
py/desispec/test/test_sky.py::TestSky::test_subtract_sky
py/desispec/test/test_sky.py::TestSky::test_uniform_resolution
  /global/common/software/desi/perlmutter/desiconda/20230111-2.1.0/conda/lib/python3.10/site-packages/numpy/core/_methods.py:189: RuntimeWarning: invalid value encountered in double_scalars
    ret = ret.dtype.type(ret / rcount)

py/desispec/test/test_calibfinder.py::TestCalibFinder::test_init
  /global/common/software/desi/perlmutter/desiconda/20230111-2.1.0/conda/lib/python3.10/site-packages/astropy/time/core.py:2798: TimeDeltaMissingUnitWarning: Numerical value without unit or explicit format passed to TimeDelta, assuming days
    warn(

py/desispec/test/test_calibfinder.py::TestCalibFinder::test_init
  /global/common/software/desi/perlmutter/desiconda/20230111-2.1.0/conda/lib/python3.10/site-packages/astropy/utils/iers/iers.py:1142: IERSStaleWarning: leap-second file is expired.
    warn("leap-second file is expired.", IERSStaleWarning)

py/desispec/test/test_extract.py::TestExtract::test_extract
  /global/common/software/desi/perlmutter/desiconda/20230111-2.1.0/conda/lib/python3.10/site-packages/numba/cuda/dispatcher.py:488: NumbaPerformanceWarning: Grid size 16 will likely result in GPU under-utilization due to low occupancy.
    warn(NumbaPerformanceWarning(msg))

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
===================================================== 247 passed, 7 skipped, 2 xfailed, 9 warnings in 269.19s (0:04:29) =====================================================

@moustakas moustakas requested a review from sbailey January 3, 2024 19:47
@@ -26,7 +26,7 @@ def get_qa_params() :
"""
global _qa_params
if _qa_params is None :
param_filename =resource_filename('desispec', 'data/qa/qa-params.yaml')
param_filename = resources('desispec').joinpath('data/qa/qa-params.yaml')
Copy link
Contributor

Choose a reason for hiding this comment

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

as evidence to not trust the coverage of our unit tests, I'm pretty sure that this should have been resources.files('desispec') instead of resources('desispec'). I may need to run a mini-prod with this branch before merging to make sure it fully works.

@sbailey
Copy link
Contributor

sbailey commented Jan 4, 2024

Update: I'm running a mini-prod on this, but my first attempt failed due to unmasked raw data problems in my test sample which we previously were more robust to for reasons completely unrelated to this PR. I'm working on constructing a different mini-prod now.

@sbailey
Copy link
Contributor

sbailey commented Jan 5, 2024

The mini-prod caught one more resources typo, which is now fixed.

For the record, a resources.files problem/challenge that I have encountered in the past is that it returns a PosixPath which acts similar to a str but isn't a strict superset of functionality, so things like filename.replace('.fits', '.csv') don't work. But I didn't find any cases of that kind of problem here, so this looks good. Thanks for the updates, merging now.

@sbailey sbailey merged commit cff7f3d into main Jan 5, 2024
24 checks passed
@sbailey sbailey deleted the importlib-over-pkg_resources branch January 5, 2024 00:30
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.

pkg_resources deprecated; use importlib.resources instead
2 participants