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

simplify installation requirements #2183

Merged
merged 8 commits into from Feb 28, 2024
Merged

simplify installation requirements #2183

merged 8 commits into from Feb 28, 2024

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Feb 22, 2024

This PR moves some imports around to simplify the dependencies needed to do basic operations with DESI spectra:

  • desispec.io.read_spectra
  • filter spectra by desitarget masks
  • desispec.coaddition.coadd and .coadd_cameras
  • desispec.spectra.stack
  • desispec.io.write_spectra

These actions can be done with a minimal environment of:

conda create -n desilite --yes python=3.10 astropy scipy numba pytest ipython
conda activate desilite
pip install fitsio

python -m pip install git+https://github.com/desihub/desiutil.git
python -m pip install git+https://github.com/desihub/desitarget.git
python -m pip install git+https://github.com/desihub/desispec.git@desilite

(after merging that @desilite branch specification won't be needed).

e.g. you no longer need specter, speclite, or desimodel to do those basic tasks.

py/desispec/test/test_lite.py is a minimal test of those spectra tasks, and a new github actions workflow installs just the minimal environment and runs just that test.

numba is the remaining unfortunate extra requirement. This is because module-level
@numba.jit decorators can't be hidden by moving numba imports inside the functions that
need numba, even if those functions are never called. We could work around this by creating
a fake numba alternative with fakenumba.jit = contextlib.nullcontext etc., but I'd like to think
a bit more about the design of that, and consider adding it to desiutil instead of desispec.
So for now, numba is still required.

@stephjuneau @moustakas @akremin et al are there any other "basic end-user functions" that you
think we should support with a minimal environment?

@weaverba137
Copy link
Member

A few suggestions:

  • I recommend selecting specific versions for desiutil and desitarget.
  • Numpy 2.0 is due out soon, so these instructions should probably select more specific versions of the conda packages as well. Unfortunately, I don't think we're ready for the major changes in Numpy 2.0 yet.
  • Does this work in both NERSC and non-NERSC environments?

@weaverba137
Copy link
Member

What about specutils? specutils 1.13.0 now supports reading DESI spectra and coadd files, and there are several notebooks that use it.

@sbailey
Copy link
Contributor Author

sbailey commented Feb 22, 2024

I recommend selecting specific versions for desiutil and desitarget.

I considered pinning, and specifically chose not to, since I think that mimics what most end users would want to do: install the latest version of the fewest possible packages. It additionally provides some coverage of any changes in desiutil or desitarget that would introduce new required 3rd party libraries, though admittedly this test wouldn't catch it until there was also a desispec change to trigger these tests.

Documentation could still recommend specific tagged versions, but I think the test should be on the latest versions of everything so that we catch new dependency requirements ASAP. For context, the NERSC nightly tests catch cross-package inconsistencies and completely new dependencies, but that is running in a full environment so doesn't really test this minimal dependency case.

Numpy 2.0 is due out soon, so these instructions should probably select more specific versions of the conda packages as well. Unfortunately, I don't think we're ready for the major changes in Numpy 2.0 yet.

Good point. I think that implies that pip installations should use

python -m pip install "numpy<2.0"

and conda installations should use

conda create -n desilite --yes python=3.10 astropy scipy "numpy<2.0" numba pytest ipython

until we are ready to support numpy 2.0.

Does this work in both NERSC and non-NERSC environments?

Yes. I developed this on my laptop, the github actions demonstrate it elsewhere, and I also tested it at NERSC starting with "module load python" instead of configuring a DESI environment (though at NERSC I would expect most users to use desiconda or cosmodesi environments, and not roll-their-own minimal environment).

What about specutils?

I think that's a different use case than I'm addressing here. IIUC specutils can read DESI spectra without having to install any DESI code, which is useful but just different. The operations like coadding, filtering, and stacking require the DESI code but don't require specutils, thus it wasn't on the minimal environment list.

There is a long tail of other things users might want to install and/or do, but here I'm trying to work around unnecessary dependencies for basic tasks, like previously needing to install a highly optimized spectroperfectionism extraction code specter just to read+coadd+stack DESI spectra due to import side effects.

@weaverba137
Copy link
Member

@sbailey, I agree with you mostly above, but what actually happens when you do

python -m pip install git+https://github.com/desihub/desiutil.git

I don't actually know what version results from that. I've never not specified a version.

If it is the latest tag, then that's fine. If it is effectively main, then it becomes harder for external users to communicate the version of these packages when reporting problems.

For context, astropy strongly encourages users to report not only the version of astropy, but of all dependencies, numpy, scipy, etc.

@sbailey
Copy link
Contributor Author

sbailey commented Feb 22, 2024

pip install git without specifying a branch/tag installs the latest commit of the default branch, i.e. main. I don't know of a way to specify "the latest tag" without hardcoding what that tag is and having to keep that in sync for docs, though that "latest tag on github" would be handy if anyone knows syntax for that.

At the same time, I think that is a documentation issue for what we tell people to install and what information to include when reporting issues. For the purposes of this PR and testing, I think installing latest main is still the right thing to do.

@weaverba137
Copy link
Member

Good to know. OK to go with that for now.

@sbailey sbailey merged commit 5229a27 into main Feb 28, 2024
26 checks passed
@sbailey sbailey deleted the desilite branch February 28, 2024 23:39
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.

None yet

2 participants