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

Add more npe2 technical references from the autogenerated content #604

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

willingc
Copy link
Contributor

References and relevant issues

Description

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Feb 28, 2025
@willingc
Copy link
Contributor Author

willingc commented Mar 9, 2025

@psobolewskiPhD @melissawm I don't think that we should fail CI on docs warnings.

@willingc willingc marked this pull request as ready for review March 9, 2025 17:38
@psobolewskiPhD
Copy link
Member

I think I rather keep em? Usually they do indicate something important.
I think they're legit here too?
See https://output.circle-artifacts.com/output/job/0a20dff1-fcb7-45b7-800d-473903bdd9ea/artifacts/0/docs/docs/_build/html/plugins/technical_references/index.html
And try to click on any of the tiles -- they don't link.
Also, the new content isn't in the ToC.

If there is something totally spurious that we know is safe to ignore, we can filter it in conf.py

Copy link
Member

@psobolewskiPhD psobolewskiPhD left a comment

Choose a reason for hiding this comment

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

In addition to addressing the link issues, I think we need to give this a bit more thought.
I like the idea of making the various contributions easier to access -- many plugins will just use one or two of them.
However, contributions.md already contains sections on those contributions, see:
https://github.com/napari/npe2/blob/main/_docs/templates/_npe2_contributions.md.jinja
I think it might be cleaner to have the new tiles use anchors that link within that existing file rather than creating more files?

Also the landing is a bit busy now. I think I would prefer a distinct top two tile set of Manifest and Contributions, as the overall guides.
And then maybe underneath a section with commonly accessed contributions -- the new tiles -- that link to the anchors in contributions.md

@willingc
Copy link
Contributor Author

willingc commented Mar 9, 2025

@psobolewskiPhD There's two schools of thought here maybe more. Working off the assumption that the warnings are unrelated to the specific PR changes. I would still display all the warnings at the end of the CI run.

  • Fix all the warnings and make sure that CI builds are always green and not flaky. (Personal opinion: we're not here yet. Our CI is slow with all the builds of examples and videos, and it is super annoying as a contributor to not be able to retrigger CI when you get to the end.)
  • Alternatively, you log the warnings as issues and work down the list over time. (Personal opinion: I would prefer we do this until after 0.6.0 is production released. If we can demonstrate highly reliable (non flaky) CI before then, we can reenable warnings as errors.)

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Mar 9, 2025

Working off the assumption that the warnings are unrelated to the specific PR changes.

Just based off of the past couple PRs (this one, Draga's PR, my last PR) where warnings blocked a merge, this hasn't really been the case. The warnings identified real issues, even though the HTML was built successfully. I think these kinds of things can be hard to catch otherwise.

So my vote would be to keep it, but I admit I am very risk averse. Happy to be over-ruled, but I think the current approach lowers maintenance burden. I can see though that it can cause stress for contributors, so if we think the cost benefit is overall not favorable, we can change it.

@willingc
Copy link
Contributor Author

willingc commented Mar 10, 2025

Hmm...I really wish that we didn't need to rebuild the gallery each run. It's very time consuming, and it has been flaky whether legit errors or not. I'm not sure what the right solution is Peter. Somewhere between status quo and temporarily removing the fail on warnings.

I'm okay with being conservative. Right now, I'm not confident in the workflow and feel it is a barrier to onboarding new contributors. For myself, I am getting into the habit of writing content putting the PR up expecting the CI to fail on something unrelated to the text changes, coming back to the PR a day or two later hoping that someone has restarted the CI, hoping it runs to green, and then someone notices and adds the ready to merge label. This workflow has prevented me from writing more edits since it's frustrating to wait this long when I am used to creating content at a much faster rate.

@melissawm
Copy link
Member

melissawm commented Mar 10, 2025

I understand both positions here, and I want to add perspective - we turned on the fail on docs option because we were having multiple PRs followed by broken links, new docs not being added to the ToC etc (see e.g. #227)

Also, I think CI has been particularly flaky lately since the new dependency groups were added and this may be a temporary situation. I do understand the frustration though, and would like to avoid it if possible.

Perhaps it would be reasonable to have make html-noplot for PR previews, and only run the full gallery on merge OR if actual gallery code is touched on the napari/napari side?

@willingc
Copy link
Contributor Author

@melissawm Wise solution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants