desihub / desispec

DESI spectral pipeline
BSD 3-Clause "New" or "Revised" License
35 stars 24 forks source link

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

Closed moustakas closed 7 months ago

moustakas commented 7 months ago

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) =====================================================
sbailey commented 7 months ago

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 commented 7 months ago

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.