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

Update tmpdir usage to tmp_path and enable no:legacypath #64

Merged
merged 3 commits into from
Feb 29, 2024

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Feb 28, 2024

Usage of tmpdir in ci_watson prevents packages that use ci_watson from using the -p no:legacypath pytest option to enforce tmp_path instead of tmpdir usage.

Although _jail is clearly marked with a leading underscore it is used in downstream:
https://github.com/search?q=repo%3Aspacetelescope%2Fjwst%20_jail&type=code
https://github.com/search?q=repo%3Aspacetelescope%2Fromancal%20_jail&type=code

This PR updates this package to use tmp_path instead of tmpdir and enables the -p no:legacypath option.

The failure of the slow-devdeps CI job also occurs on main and appears related to numpy 2.0.

@braingram braingram changed the title Tmp path Update tmpdir usage to tmp_path and enable no:legacypath Feb 28, 2024
Copy link
Collaborator

@pllim pllim left a comment

Choose a reason for hiding this comment

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

LGTM but @jhunkeler wrote this so maybe he can confirm.

@pllim
Copy link
Collaborator

pllim commented Feb 28, 2024

tox.ini needs to be updated to make devdeps pass. Add this. GitHub won't let me do it for some reason.

devdeps: pyerfa>=0.0.dev0

try:
yield tmpdir.strpath
yield str(tmp_path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You may want to just yield the tmp_path instead of its string form?

That is what is done here

https://github.com/astropy/astroquery/blob/4d2311745752ffc0133e439053dd4046e4a8294b/astroquery/conftest.py#L70-L78

in a similar fixture I wrote based on _jail.

This would allow one to be sure one is pathlib compatible, though it might break a few things downstream. Though perhaps the breaking is good?

Or one could leave this fixture unchanged and make a properly-named fixtured called tmp_cwd that can be used that yields a pathlib.Path object, and can be transitioned to at anytime by any package that uses ci-watson.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the question and comments.

I added the str to keep this consistent for the downstream (as you mentioned changing it to a Path might break things).

Given the naming (_jail) I would have assumed this was not intended for "public" use. So replacing it with a public fixture (or having the downstream libraries implement their own for their needs) seems reasonable.

To use -p no:legacyapth here (to keep this test suite updated) _jail would need to be updated (as is done in this PR).

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this package, this fixture is also used for HSTCAL. So unless you feel like patching up hstcal and acstools test suite, backward compatibility is a must.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Definitely not! Let's keep backwards compatibility. 😅

@pllim
Copy link
Collaborator

pllim commented Feb 28, 2024

Oops sorry, also have to add this to PIP_EXTRA_INDEX_URL

https://pypi.anaconda.org/liberfa/simple

@braingram
Copy link
Contributor Author

Oops sorry, also have to add this to PIP_EXTRA_INDEX_URL

https://pypi.anaconda.org/liberfa/simple

Thanks for your help! CI is now green.

I'm not really seeing any numpy usage (grepping finds no imports of numpy) in this package so maybe numpy 2.0 testing isn't needed? That's certainly outside the scope of this PR.

setup.cfg Outdated Show resolved Hide resolved
@pllim
Copy link
Collaborator

pllim commented Feb 28, 2024

Does CRDS use numpy/astropy?

crds

Also, if you want to test against astropy-dev, you should probably also pull in numpy-dev, but you don't have to. Your call.

@pllim pllim requested a review from jhunkeler February 28, 2024 16:46
@pllim pllim added the enhancement New feature or request label Feb 28, 2024
@pllim pllim merged commit 24e9024 into spacetelescope:main Feb 29, 2024
9 checks passed
@pllim
Copy link
Collaborator

pllim commented Feb 29, 2024

Thanks, all!

@braingram braingram deleted the tmp_path branch February 29, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants