braingram
commented
3 months ago This is duplicate of https://github.com/spacetelescope/jwst/pull/8565
Closed jhunkeler closed 3 months ago
braingram
commented
3 months ago This is duplicate of https://github.com/spacetelescope/jwst/pull/8565
hbushouse
commented
3 months ago Because this is a duplicate of #8565 I'll close this one and we'll merge the other.
jhunkeler
commented
3 months ago Sorry, I should have explained my motives here. This PR targeted the release branch in order to fix the RTD build without affecting development. I thought perhaps we wanted to keep master open to numpy 2.x related errors because it was released yesterday and there's no going back, so to speak.
braingram
commented
3 months ago Thanks @jhunkeler I missed that this is targeting the release branch. Is the plan to make a 1.14.1 with the pin?
braingram
commented
3 months ago I think at this point we have to pin numpy on main since our dependencies do not have compatible builds. Otherwise we can't run the test suite and evaluate PRs. Ideally we can remove the 2.0 pin from main before the next release but I think that depends largely on if opencv makes a release or if we can remove it as a dependency.
I'm open to ideas, let me know if it's helpful to chat about this.
jhunkeler
commented
3 months ago I wasn't planning on making a 1.14.1 release because 1.14.0's requirements-sdp.txt already pins numpy to 1.26.4. It's just that RTD doesn't use the requirements files (only pip install -e .[docs]).
hbushouse
commented
3 months ago I also missed the fact that this is targeting the release branch, not master. Reopening.
braingram
commented
3 months ago If this is only for the docs would the rtd_environment dependencies be a good spot:
https://github.com/spacetelescope/jwst/blob/380dc5eb082c6c5d4e024a9337999f69cd5b8443/docs/rtd_environment.yaml#L5
jhunkeler
commented
3 months ago I started with the rtd_environment.yaml but abandoned the approach. The pip installation command would likely upgrade the package to numpy 2.x anyway.
python -m pip install --upgrade --upgrade-strategy only-if-needed --no-cache-dir .[docs]
I haven't tested the theory locally though.
jhunkeler
commented
3 months ago I agree. My scope is a little too narrow... Making a 1.14.1 release is the best option we have to avoid disruption to any workflows in the wild.
jhunkeler
commented
3 months ago re: https://github.com/spacetelescope/jwst/pull/8566#issuecomment-2173611997
Adding numpy to the yaml configuration does work. The major downside will be remembering to keep this aligned with whatever is pinned by pyroject.toml and the requirements-*.txt files.
docs/rtd_readthedocs.yaml
name: rtd311
channels:
- conda-forge
- defaults
dependencies:
- python=3.11
- pip
- graphviz
- sphinx_rtd_theme>1.2.0
- pip:
- numpy<2$ mamba env create -f docs/rtd_environment.yaml
$ mamba activate rtd311
$ python -m pip install --upgrade --upgrade-strategy only-if-needed --no-cache-dir '.[docs]'
$ pip freeze | grep numpy=
numpy==1.26.4
$ cd docs
$ python -m sphinx -T -W --keep-going -b html -d _build/doctrees -D language=en . output/html
# ...
build succeeded.
The HTML pages are in output/html.So does this PR need modification yet, in order to implement the change in rtd_readthedocs instead of pyproject.toml? And do we also still need to create a 1.14.1 release with numpy pinned to <2.0 to protect pip installers?
braingram
commented
3 months ago Adding numpy to the yaml configuration does work. The major downside will be remembering to keep this aligned with whatever is pinned by pyroject.toml and the requirements-*.txt files.
Thanks for looking into it. Given this and your other comments adding the pin to the pyproject.toml sounds good to me.
So does this PR need modification yet, in order to implement the change in rtd_readthedocs instead of pyproject.toml? And do we also still need to create a 1.14.1 release with numpy pinned to <2.0 to protect pip installers?
This PR looks good to me the way it is. A patch release will be needed with the pin to protect pip installers (I tested this locally).
I'm not sure what the procedure for a patch release would be. Specifically:
zacharyburnett
commented
3 months ago since 1.14.1 would be a PyPI-only release, I don't think it needs to be included in the metadata on main (the table of DMS builds in README.md, CITATION.cff, etc.) but we'll need to remember to use the next version number for the next release
drlaw1558
commented
3 months ago It may not be a DMS build, but nonetheless the table in README.md is a go-to place for finding the version history. I'd suggest doing the same thing as with 1.13.4 and 1.12.4 and adding an entry with a note "PyPI-only release for external users"
zacharyburnett
commented
3 months ago It may not be a DMS build, but nonetheless the table in README.md is a go-to place for finding the version history. I'd suggest doing the same thing as with 1.13.4 and 1.12.4 and adding an entry with a note "PyPI-only release for external users"
sure thing, here: https://github.com/spacetelescope/jwst/pull/8617
This should rectify the following RTD build error on
stable(aka1.14.0):Pinning
numpy<2locally does not produce any errors: