Skip to content

DOC [PST] disable gallery link tweaks when html-noplot to avoid warnings #28512

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 5 commits into from
Mar 12, 2024

Conversation

Charlie-XIAO
Copy link
Contributor

Please note that this PR targets the new_web_theme branch!

Towards #28084. For context see #28331 (comment). In short, when built without running gallery examples (make html-noplot) the badge links and download links could be missing; we do not want to tweak them then. This PR also improves the warning messages a bit.

@ogrisel

Copy link

github-actions bot commented Feb 23, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 754b8d4. Link to the linter CI: here

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

@@ -141,22 +145,31 @@ def _create_badge_link(link):
# These will be removed at the end if new links are successfully created
sg_note = soup.find("div", class_="sphx-glr-download-link-note")
sg_footer = soup.find("div", class_="sphx-glr-footer")
assert sg_note is not None, "Failed to find the top note"
assert sg_footer is not None, "Failed to find the gallery footer"
Copy link
Member

Choose a reason for hiding this comment

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

I have the feeling that those assertions might still fail if we run some examples, but not all via the EXAMPLES_PATTERN env variable: https://scikit-learn.org/stable/developers/contributing.html#building-the-documentation

Can you please check?

Copy link
Contributor Author

@Charlie-XIAO Charlie-XIAO Feb 23, 2024

Choose a reason for hiding this comment

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

Ah didn't notice that because in make.bat we did not have this option. I will take a look (maybe a bit later).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed everything and built from scratch on my local machine with html-noplot, but it seems that I did not get those errors?

Then I looked at the sphinx-gallery source code: in the save_rst_example function no matter whether the example is run or not the components we tweak will be added.

Is it possible that we are using different versions of sphinx-gallery (mine is 0.15.0)? Or can you please provide more context on your side @ogrisel?

Copy link
Member

Choose a reason for hiding this comment

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

Let me try again with a clean environment and recent versions of everything.

Copy link
Member

@ogrisel ogrisel Mar 11, 2024

Choose a reason for hiding this comment

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

So I created a new env with the latest sphinx-gallery (0.15.0) and ran make html-noplot and I can reproduce the WARNING: Failed to tweak gallery links in... warnings I initially reported in #28331 (comment).

Also, I tried this branch and the warning go away. But it seems that I can no longer get the EXAMPLES_PATTERN thing to work. For instance I tried EXAMPLES_PATTERN=".*mean.*" make html-noplot and I did not get the plot generated in plot_kmeans_stability_low_dim_dense.html. I don't remember how it's supposed to work. But doing experiment is very slow. It seems that make html-noplot is much slower than it used to be. We would need to profile to find why it's the case. Note: I have not yet tried on main, maybe the slowness is already there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I created a new env with the latest sphinx-gallery (0.15.0) and ran make html-noplot and I can reproduce the WARNING: Failed to tweak gallery links in... warnings I initially reported in #28331 (comment).

Thanks for experimenting! Then there might some mistakes made on my side. I will take another look into it some time.

Also, I atried this branch and the warning go away. But it seems that I can no longer get the EXAMPLES_PATTERN thing to work. For instance I tried EXAMPLES_PATTERN=".*mean.*" make html-noplot and I did not get the plot generated in plot_kmeans_stability_low_dim_dense.html. I don't remember how it's supposed to work.

Hmm that's strange. I don't think it's likely caused by this PR because tweak_gallery_links is just a post transform that executes in the very last step while plot generation is determined very early even before page generation. I will try to test this as well.

But doing experiment is very slow. It seems that make html-noplot is much slower than it used to be. We would need to profile to find why it's the case. Note: I have not yet tried on main, maybe the slowness is already there.

I also noticed the significant slowdown (as long as using pydata-sphinx-theme): not 100% sure but it may be caused by pydata/pydata-sphinx-theme#1643.

@Charlie-XIAO
Copy link
Contributor Author

Thinking over again, I think we may not really need to care about all those corner cases simply because there are too many of them. For instance, make html-noplot, filename_pattern, ignore_pattern, run_stale_examples, etc. and the case is different when no plot is generated versus some/all plots are already generated (they will not be overwritten unless run_stale_examples but run_stale_examples can also run for only part of the gallery).

My current idea is: If we can tweak then we do it, otherwise leave it as is (without warning). The thing is, at worst we failed to tweak but since the links will still be there (in the original position) it is not really a big issue. Moreover, if this post transform really fails some day, most probably all pages will fail (instead of only some pages failing) then it is easy to discover by just looking at a random gallery page in the website. WDYT @ogrisel?

@ogrisel
Copy link
Member

ogrisel commented Mar 12, 2024

My current idea is: If we can tweak then we do it, otherwise leave it as is (without warning).

Let's do the latter.

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

I tested the new state of the PR with make html-noplot and I confirm that I don't get the warnings anymore.

LGTM!

@ogrisel ogrisel merged commit 3400749 into scikit-learn:new_web_theme Mar 12, 2024
@Charlie-XIAO Charlie-XIAO deleted the pst-tweak-gal-noplot branch March 12, 2024 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants