Skip to content

Add optional dependency sections for gallery and docs #7487

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

Merged
merged 10 commits into from
Feb 20, 2025

Conversation

jni
Copy link
Member

@jni jni commented Jan 4, 2025

Looking for feedback here, especially from @Czaki and @melissawm.

Prompted by this comment from @psobolewskiPhD.

When adding an example to the gallery, any new dependencies need to be added to
both (currently nonexistent) docs and testing optional dependency groups.
That made me think that there was something a bit off with how we were thinking
about things. I thought about this a bit more and thought it best to create a
"gallery" group, which both testing and docs depend on.

I've implemented this in this PR, adding [docs] and [gallery] and then adding
dependencies on [gallery] to both [docs] and [testing]. Some questions:

  • Currently, I've added anything that is imported in the gallery to the gallery
    dependencies, even if it is already a core dependency. The rationale here is
    that if we, for example, drop the dependency on pandas, we still want the
    examples using pandas to run. It's nice to have our core dependencies and our
    direct example dependencies uncoupled.
  • However, adding minimum versions to the examples makes it quite onerous to
    keep gallery dependencies in sync. I've therefore kept gallery dependencies
    bare, with no version requirements. Is that a bad idea?
  • The docs dependencies are copied over from the requirements.txt file in the
    docs repo
    .
    Is that what we want to do or do we want to clean these up/add
    minimum/maximum versions? (And maybe that could come in a separate PR?)

@melissawm
Copy link
Member

I'd love to see this for 0.6 👀

Comment on lines +175 to +193
docs = [
"napari[gallery]",
"sphinx<8",
"sphinx-autobuild",
"sphinx-tabs",
"sphinx-tags",
"sphinx-design",
"sphinx-external-toc",
"sphinx-favicon>=1.0",
"sphinx-copybutton",
"sphinx-gallery",
"sphinx_autodoc_typehints==1.12.0",
"myst-nb",
"napari-sphinx-theme>=0.3.0",
"matplotlib",
"lxml[html_clean]>5",
"imageio-ffmpeg",
"pytest",
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

With this change, we need to update upgrade dependencies' workflow.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Czaki can you elaborate on this? What do we need to do?

Copy link
Collaborator

@Czaki Czaki Feb 14, 2025

Choose a reason for hiding this comment

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

minimaly change this line

uv pip compile --python-version 3.10 --upgrade --output-file ${constraints}/constraints_py3.10_docs.txt ${pyproject_toml} ${constraints}/version_denylist.txt ${constraints}/version_denylist_examples.txt docs/requirements.txt "${flags[@]}"

to replace docs/requirements.txt with --extra docs

Fully. We do not need to clone two repos for this action, if we store dependencies in pyproject.toml. So we may refactor upgrade_test_constraints.yml to clone one repo, so simplify run it locally. But not do it as it will conflict with #7603. I prefer separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in ad9fc01

@jni jni added this to the 0.6.0 milestone Feb 14, 2025
@Czaki
Copy link
Collaborator

Czaki commented Feb 14, 2025

We should also refactor circleci configuaration

- run:
name: Install napari-dev
command: |
. venv/bin/activate
python -m pip install -e "napari/[pyside,dev]"
environment:
PIP_CONSTRAINT: napari/resources/constraints/constraints_py3.10_docs.txt
- run:
name: Build docs
command: |
. venv/bin/activate
cd docs
xvfb-run --auto-servernum make docs
environment:
PIP_CONSTRAINT: ../napari/resources/constraints/constraints_py3.10_docs.txt

And this https://github.com/napari/docs/blob/20829cff8286f291f7d20d792c876137200f7c4e/Makefile#L24

@jni
Copy link
Member Author

jni commented Feb 14, 2025

@Czaki any thoughts on:

However, adding minimum versions to the examples makes it quite onerous to
keep gallery dependencies in sync. I've therefore kept gallery dependencies
bare, with no version requirements. Is that a bad idea?

@Czaki
Copy link
Collaborator

Czaki commented Feb 14, 2025

@Czaki any thoughts on:

However, adding minimum versions to the examples makes it quite onerous to
keep gallery dependencies in sync. I've therefore kept gallery dependencies
bare, with no version requirements. Is that a bad idea?

Do you mean keep the requirements.txt in docs repo? We already have pins in the main repository, so relaxing any upper constraints requires updating the main repository to have it working

@willingc willingc added the needs:out-of-draft Needs transition from draft to ready for review label Feb 17, 2025
@jni
Copy link
Member Author

jni commented Feb 18, 2025

Do you mean keep the requirements.txt in docs repo? We already have pins in the main repository, so relaxing any upper constraints requires updating the main repository to have it working

No, requirements.txt goes. I mean, for the gallery dependencies, not specifying version ranges at all in pyproject.toml.

@github-actions github-actions bot added the maintenance PR with maintance changes, label Feb 18, 2025
@jni jni marked this pull request as ready for review February 18, 2025 07:34
@jni jni requested a review from a team as a code owner February 18, 2025 07:34
@jni jni removed the needs:out-of-draft Needs transition from draft to ready for review label Feb 18, 2025
@Czaki
Copy link
Collaborator

Czaki commented Feb 18, 2025

No, requirements.txt goes. I mean, for the gallery dependencies, not specifying version ranges at all in pyproject.toml.

Why did you want to keep two sources of truth? Why keep docs dependency in two places?

Currently, I've added anything that is imported in the gallery to the gallery
dependencies, even if it is already a core dependency. The rationale here is
that if we, for example, drop the dependency on pandas, we still want the
examples using pandas to run. It's nice to have our core dependencies and our
direct example dependencies uncoupled.

I do not agree with this assumption. We run tests on CI to spot such things. And having dependency in multiple places will only increase maintenance jobs. If some dependency needs to be added in multiple places, someone will forget to add them in one of them.

I think that PEP 735 is something that we should investigate https://peps.python.org/pep-0735/

@jni
Copy link
Member Author

jni commented Feb 18, 2025

No, requirements.txt goes. I mean, for the gallery dependencies, not specifying version ranges at all in pyproject.toml.

Why did you want to keep two sources of truth? Why keep docs dependency in two places?

There's not two sources. The docs dependencies are needed to build the docs, the gallery dependencies are needed for the gallery. They are different sections because you don't need all the docs deps for the gallery, and specifically you need the gallery dependencies for testing but you don't need sphinx for testing. So we can't put the gallery dependencies in [docs].

@jni
Copy link
Member Author

jni commented Feb 18, 2025

@Czaki any idea what's going on here?

 Downloaded pyqt5-qt5
  × Failed to build `zeroc-ice==3.6.5`
  ├─▶ The build backend returned an error
  ╰─▶ Call to `setuptools.build_meta:__legacy__.build_wheel` failed (exit
      status: 1)

      [stdout]
      running bdist_wheel

@jni
Copy link
Member Author

jni commented Feb 18, 2025

I do not agree with this assumption. We run tests on CI to spot such things. And having dependency in multiple places will only increase maintenance jobs. If some dependency needs to be added in multiple places, someone will forget to add them in one of them.

Ok, I can remove the redundant deps. 👍

@Czaki
Copy link
Collaborator

Czaki commented Feb 18, 2025

@Czaki any idea what's going on here?

 Downloaded pyqt5-qt5
  × Failed to build `zeroc-ice==3.6.5`
  ├─▶ The build backend returned an error
  ╰─▶ Call to `setuptools.build_meta:__legacy__.build_wheel` failed (exit
      status: 1)

      [stdout]
      running bdist_wheel

Zeroc-ice do not provide linux wheels and fail to build from source because of lack of build dependencies.
I do not know from where zeroc-ice comes

@willingc
Copy link
Collaborator

willingc commented Feb 18, 2025

@Czaki :

help: `zeroc-ice` (v3.6.5) was included because `omero-py` (v5.15.0) depends
        on `zeroc-ice`

Can we bump omero-py? It's a pretty old release before 3.12 support. https://github.com/ome/omero-py/blob/master/CHANGELOG.md#5190-february-2024

@jni
Copy link
Member Author

jni commented Feb 18, 2025

The CI that failed is 3.10, and the current release only supports 3.11 and 3.12. I remember now why we made examples/3d_kymograph_.py not run or be tested automatically (hence the trailing underscore in the filename). 😅 We might be able to update it to use an ome-zarr dataset in the future, but for now, I've just removed the dependency — that file isn't run anyway.

Copy link

codecov bot commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.84%. Comparing base (3a46b9c) to head (93e3084).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7487      +/-   ##
==========================================
- Coverage   92.87%   92.84%   -0.04%     
==========================================
  Files         629      629              
  Lines       58971    58971              
==========================================
- Hits        54771    54749      -22     
- Misses       4200     4222      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@psobolewskiPhD
Copy link
Member

FYI The easiest way to get omero working is via conda-forge.
zeroc-ice 3.6.5 now has binaries for python 3.11 and 3.12

@jni
Copy link
Member Author

jni commented Feb 19, 2025

We run tests on CI to spot such things. And having dependency in multiple places will only increase maintenance jobs. If some dependency needs to be added in multiple places, someone will forget to add them in one of them.

@Czaki I removed the core dependencies from the gallery dependencies. This should be ready to merge. 🤞

@jni
Copy link
Member Author

jni commented Feb 19, 2025

@Czaki investigating the nilearn issue, it seems that it's ok to keep it in the gallery dependencies — the new version is not incompatible with current NumPy. Looking at #5716, I think that I should rename the file to not include a trailing underscore (so that it gets added to the gallery in the docs), remove the ModuleNotFound checks, and remove version_denylist_gallery. I would rather do this in a follow-up PR. Do you agree with this plan? (If so then this can be merged.)

@Czaki
Copy link
Collaborator

Czaki commented Feb 19, 2025

I still suggest updating:

napari/tox.ini

Lines 133 to 142 in 3a46b9c

[testenv:py{310,311,312,313}-{linux,macos,windows}-{pyqt5,pyside2,pyqt6,pyside6}-examples-{cov,no_cov}]
deps =
# For surface_timeseries_.py example
nilearn
# Due to nilearn incompatibility with numpy >=1.24 constraint numpy and add packaging for version check
packaging
commands =
cov: coverage run --parallel-mode \
!cov: python \
-m pytest napari/_tests/test_examples.py -v --color=yes --basetemp={envtmpdir} {posargs}

to

[testenv:py{310,311,312,313}-{linux,macos,windows}-{pyqt5,pyside2,pyqt6,pyside6}-examples-{cov,no_cov}]
commands =
    cov: coverage run --parallel-mode \
    !cov: python \
        -m pytest napari/_tests/test_examples.py -v --color=yes --basetemp={envtmpdir} {posargs}

As all gallery extras are now in test dependecies.

I think that I should rename the file to not include a trailing underscore (so that it gets added to the gallery in the docs), remove the ModuleNotFound checks, and remove version_denylist_gallery. I would rather do this in a follow-up PR. Do you agree with this plan? (If so then this can be merged.)

I think that making these changes in separate PR is a good idea.

@jni
Copy link
Member Author

jni commented Feb 19, 2025

I still suggest updating:

Ah, I didn't fully grok that. Pushed! Let's just wait for CI now. 🙏

@jni jni added the ready to merge Last chance for comments! Will be merged in ~24h label Feb 19, 2025
@jni jni merged commit c3fdd6c into napari:main Feb 20, 2025
42 checks passed
@jni jni deleted the more-optional-deps branch February 20, 2025 01:59
@github-actions github-actions bot removed the ready to merge Last chance for comments! Will be merged in ~24h label Feb 20, 2025
jni pushed a commit that referenced this pull request Feb 23, 2025
# References and relevant issues
Part of: napari/docs#589

# Description
In #7487 a few requirements were
missed.
This PR adds them back.
I also update the `_docs` constraints -- not for any of the new versions
(leave that to the normal PR), just for docs.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
jni pushed a commit that referenced this pull request Feb 27, 2025
# Description

There are three changes here.
1. stop cloning docs repo as we are using extension group
(#7487).
2. change directory to napari repo so commands on the top of constraints
files
napari/resources/constraints/constraints_py3.10.txt(

https://github.com/napari/napari/blob/39b385d23e9f0af40dcfea205d7e3a5d52feedb8/resources/constraints/constraints_py3.10.txt#L2
could be easier executed locally as there will be no napari_repo in the
path. That will allow to easier recalculate constraints locally.
3. remove PyQt5 from constraints, as there is no single PyQt5 version
that provides wheels for both windows and macOS ARM. This simplifies
maintenance and local bumping. As PyQt5 is in maintenance mode, I do not
expect that PyQt5 may break anything and removing it from constraints
allows removing separate Windows files.

With these changes, it will be easier to run update of constraints
locally.
willingc added a commit to napari/docs that referenced this pull request Jul 20, 2025
# References and relevant issues

closes #670

napari/napari#7642
napari/napari#7487



# Description

After the most recent updates of constraints update, we could simplify
this instrucition.

---------

Co-authored-by: Peter Sobolewski <[email protected]>
Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Constantin Aronssohn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance PR with maintance changes,
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants