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

unit test failure with astropy/5.2 #123

Closed
marcelo-alvarez opened this issue Jan 10, 2023 · 8 comments
Closed

unit test failure with astropy/5.2 #123

marcelo-alvarez opened this issue Jan 10, 2023 · 8 comments
Assignees

Comments

@marcelo-alvarez
Copy link
Contributor

python setup.py test

fails with

ImportError while loading conftest '/tmp/specsim-test-uwh0fuag/lib/python3.10/site-packages/specsim/conftest.py'.
specsim/conftest.py:15: in <module>
    from astropy.tests.plugins.display import PYTEST_HEADER_MODULES, TESTED_VERSIONS
E   ModuleNotFoundError: No module named 'astropy.tests.plugins'

when using astropy/5.2. See https://github.com/astropy/pytest-astropy-header for a possible solution.

@sbailey

@sbailey
Copy link
Contributor

sbailey commented Jan 10, 2023

@dkirkby or @weaverba137 do you have experience porting a package away from astropy.tests.plugins? I'm not excited about the solution suggested at https://github.com/astropy/pytest-astropy-header which seems to suggest installing another custom package. How would you feel about de-astropyifying the specsim unit test infrastructure?

This is a blocking factor for using astropy 5.2, which was an optional update that we were hoping to include in the DR1/Iron software environment.

@weaverba137
Copy link
Member

It depends on how far you want to go.

In this particular instance, astropy.tests.plugins.display, which is now in a separate package, was providing some additional header information for the unit tests, but not affecting the unit tests themselves.

Do you want to take the trivial step of removing this header information, or do you want to purge all Astropy infrastructure from specsim?

Historically, specsim was meant to be an Astropy-affiliated package, and there is an expectation that Astropy-affiliated packages use the Astropy infrastructure. Are we considering re-evaluating that position?

@weaverba137
Copy link
Member

@marcelo-alvarez , is there a particular branch you are working on?

@weaverba137
Copy link
Member

Let me emphasize: I am not actually advocating in favor of removing Astropy infrastructure. On the contrary, I think this is a trivial fix: just don't display those headers, they aren't needed for the actual tests.

But I do want to move forward the discussion of the long-term future of the specsim package, and who is maintaining it.

One thing I will note is that unit tests of this package are going to be difficult until we migrate to GitHub Actions. Until then we will need more details about the exact environment that @marcelo-alvarez is using that has Astropy/5.2.

@sbailey
Copy link
Contributor

sbailey commented Jan 10, 2023

PR #115 is for migrating to github actions. The blocking factor might be a maintainer who feels ownership of approving PRs and making these changes.

@weaverba137 good point about astropy-affiliated packages using astropy infrastructure, as long as we have a maintainer keeping up with that infrastructure. For now I'm happy with whatever is the easiest solution to restore unit tests for astropy 5.2 while remaining backwards compatibility with 5.0.

@weaverba137
Copy link
Member

I saw that, but we would need to be careful because PR #115 doesn't actually include tests on Astropy/5.2. so additional work would be needed to sync that with the environment that @marcelo-alvarez is using.

@dylanagreen
Copy link
Contributor

For the immediate fix of the unit test issue I will self assign and work on it. It sounds like the preferred solution here is just to not display the headers in the tests moving forward.

@dylanagreen dylanagreen self-assigned this Jan 10, 2023
@marcelo-alvarez
Copy link
Contributor Author

@dylanagreen, thanks, indeed it looked like it would be a simple fix and glad it turned out to be.

One thing I will note is that unit tests of this package are going to be difficult until we migrate to GitHub Actions. Until then we will need more details about the exact environment that @marcelo-alvarez is using that has Astropy/5.2.

@weaverba137 in case this is still useful to you in some larger context than resolving this issue, you can obtain the environment I used by typing

source /global/common/software/desi/users/malvarez/desi_environment.sh local 2.0.1

on a Perlmutter login node.

@dylanagreen dylanagreen moved this to In Progress in Iron Jan 10, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Iron Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

4 participants