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

fix install with latest desiutil without DesiTest #832

Merged
merged 12 commits into from
Jan 30, 2025
Merged

fix install with latest desiutil without DesiTest #832

merged 12 commits into from
Jan 30, 2025

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Jan 29, 2025

desihub/desiutil#212 removed desiutil.setup.DesiTest, which broke the ability to install desitarget when using the latest desiutil. This PR fixes that by removing the DesiTest import from setup.py. Now to run desitarget tests you have to use pytest and not python setup.py test. That has been our recommendation for awhile (and how github actions and NERSC tests have been performed), but this PR removes support for the old python setup.py test option.

With current main this fails with "ImportError: cannot import name 'DesiTest' from 'desiutil.setup'":

conda create --yes --name desitest 'numpy<2' 'astropy<7'
conda activate desitest

pip install git+https://github.com/desihub/desiutil.git@main
pip install git+https://github.com/desihub/desitarget.git@main

Replacing that last line with this branch works

pip install git+https://github.com/desihub/desitarget.git@fix-install

After merging this branch, it should be possible to install desitarget/main when using desiutil/main.

@sbailey sbailey requested a review from weaverba137 January 29, 2025 19:42
@sbailey
Copy link
Contributor Author

sbailey commented Jan 29, 2025

Ugh, tests are failing while installing cfitsio. Will need some more work to restore that.

Copy link
Member

@weaverba137 weaverba137 left a comment

Choose a reason for hiding this comment

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

This looks fine to me. It looks like you may need to sort out dependencies for compiling fitsio. The first thing to try is using ubuntu-22.04 instead of ubuntu-latest.

@sbailey
Copy link
Contributor Author

sbailey commented Jan 30, 2025

Well that was unsatisfying. matplotlib is trying to force an upgrade to numpy>2, which isn't compatible with the other packages. Using --no-deps for matplotlib means it doesn't have other dependencies like PIL, which I haven't figured out how to install yet given the other package constraints...

@weaverba137
Copy link
Member

I went through something very similar last week. I'll point you to it in the morning.

@weaverba137
Copy link
Member

You might want to compare with desihub/desimodel#178.

@coveralls
Copy link

coveralls commented Jan 30, 2025

Coverage Status

coverage: 53.051%. remained the same
when pulling c0b7752 on fix-install
into 5df2bfa on main.

@sbailey
Copy link
Contributor Author

sbailey commented Jan 30, 2025

Finally, success. In the end the coverage and regular unit tests have a somewhat different matrix of dependency versions because that is just how it landed, but I'll declare that to be a feature for providing some examples of combinations of versions that work.

@sbailey sbailey merged commit 374c54e into main Jan 30, 2025
10 checks passed
@sbailey sbailey deleted the fix-install branch January 30, 2025 23:52
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.

3 participants