Warning triggered for navigation_with_keys

[blink1073]

In many of the jupyter repos, we use pydata_sphinx_theme. All of our docs builds have started failing with 0.14.2 with the warning about default value of navigation_with_keys changing. Would it be possible to not trigger that warning? This seems like a lot of additional work for such a benign change. If this is going to be the policy of pydata_sphinx_theme in general, I'd like to know that as well so I can switch themes across the board.

https://github.com/jupyter/notebook/actions/runs/6634029880/job/18038165438

[12rambau]

full disclosure, the decision was taken here: #1503 (comment)

I would invite @drammock and @gabalafou to the thread as they worked on the linked PR.

I'm on @drammock side on this one, even though it's a small change, we inverted a parameter of our conf.py which will change the behaviour of all documentation relying on default values.

What we could do is raise it only if the value is not set, letting you with the easy navigation_with_keys: False to keep default behaviour without the warning.

[drammock]

What we could do is raise it only if the value is not set, letting you with the easy navigation_with_keys: False to keep default behaviour without the warning.

That is how it was supposed to work. Maybe that conf.py change will be enough to silence the warning; if not we should make it work that way and then release a hotfix

[blink1073]

In many cases we had no settings at all for the html theme. If there were a new single setting to opt out of these warnings I'd use that, otherwise this is too much of a maintenance cost for us.

[drammock]

I can confirm that setting

html_theme_options = {
    "navigation_with_keys": False,
}

in conf.py silences the warning, as intended. The warning is also silenced if you set it to True if you prefer the old default behavior. It only warns if the option is unset. After the next release, the default in theme.conf will change from unset (None) to False and the warning will be gone for good.

[drammock]

closing as the fix is entirely in user's conf.py, no changes to the theme needed. If I've missed something / the recommended fix doesn't work, please feel free to re-open.

[blink1073]

That doesn't fix my issue, we already knew that, but it confirms that we won't use this theme. Please reconsider the policy

[gabalafou]

This may be a total noob question, sorry if so, but why does Notebook use the -W option with sphinx-build? That's what is causing the doc build step in the CI to fail, correct?

I guess the thing that I don't understand here is that it seems that the theme needs a way to let users know about planned changes. But maybe it's an anti-pattern or unfriendly to CI systems to use a warning?

[blink1073]

We use -W to catch legitimate warnings from sphinx when something won't render correctly. Sphinx does not have this policy about warning about changing defaults from what I have seen. Unlike pytest, we don't have fine-grained control over which warnings to suppress. This is a theme, I trust the theme to have sensible defaults, and just update itself.

[drammock]

This is a theme, I trust the theme to have sensible defaults, and just update itself.

fair enough, and normally we don't warn about even very major changes like the recent accessibility-inspired color and style update. But this is a noticeable UX change for those who relied on it; we thought it was exceptional and so decided to warn.

If there were a new single setting to opt out of these warnings I'd use that, otherwise this is too much of a maintenance cost for us.

isn't the suggested change to conf.py providing just that? Or are you asking for some setting that will silence all warnings we might someday ever generate? We don't have such a mechanism in place.

We use -W to catch legitimate warnings from sphinx when something won't render correctly.

We do the same thing for our docs, and compare the captured warnings with a list of expected ones (there are some related to the "kitchen sink" page that we can't prevent because we don't control that page's content). You could adopt a similar strategy, and safe-list this particular warning.

[drammock]

I hear you that CIs breaking across multiple projects is a big headache and if you really want to jump ship to another theme obviously I can't stop you.

I'm reopening this issue because I'm willing to consider the perspective that warning here was inappropriate. We've had folks complain before about changes to private functions that we use in our __init__ so that influenced me to treat this more like a package (where we explictly now define a public API, and things like deprecation warnings are considered a normal good practice). I've not developed/maintained a theme prior to this one so if norms exist like "have sensible defaults and update yourself (without issuing warnings)" I am unaware of them.

Requesting @trallard @12rambau @choldgraf @jorisvandenbossche to weigh in here.

[vyasr]

I can confirm that setting

html_theme_options = {
    "navigation_with_keys": False,
}

in conf.py silences the warning, as intended. The warning is also silenced if you set it to True if you prefer the old default behavior. It only warns if the option is unset. After the next release, the default in theme.conf will change from unset (None) to False and the warning will be gone for good.

I'm afraid that this isn't the behavior we're currently observing. Here are a couple of PRs from the cudf repo where runs in the last 24 hours are showing the warning:

• First PR, along with the corresponding CI run and the output line showing the warning
• Second PR, along with the corresponding CI run and the output line showing the warning

This is happening despite the fact that we set navigation_with_keys: True explicitly in our conf.py. Are we missing something here?

[blink1073]

I think having an option like "suppress_warnings" would be enough to satisfy both sets of users. The sphinx "-w" option to write to a file and post-analyze isn't ergonomic and isn't something I want to do across all the jupyter repos.

[12rambau]

It seems there are 2 things to update before releasing a hotfix here:

• add an option to silence all warnings from the theme to allow CI to rely blindly on the default parameters
• make sure the warning is not issued when the parameter is set in conf.py

@drammock I'm happy with both of them.

This is a theme, I trust the theme to have sensible defaults, and just update itself.

@blink1073, thanks a lot for your trust but it seems many of our users are taking advantage of the customization options, and in previous releases we have received complaints for making changes in the default behaviors without notifying our users (thus the deprecation process). few examples:

• change from search icon to search field
• new search index
• move the search from the primary sidebar to the header
• add the show source to the secondary sidebar
• move the read the doc flyout to the primary sidebar

[blink1073]

@12rambau that sounds like a fair summary to me, thank you! Perhaps more precisely, we'd like a way to silence this specific class of warning (changing default or behavior). Although most other things that we'd care about should probably be errors not warnings.

[drammock]

This is happening despite the fact that we set navigation_with_keys: True explicitly in our conf.py. Are we missing something here?

@vyasr it looks like that line was added to your conf.py just yesterday (25 Oct): https://github.com/rapidsai/cudf/blame/branch-23.12/docs/cudf/source/conf.py#L109

Are you certain that rapidsai/cudf#14323 was already merged into the base branch before those PR runs for rapidsai/cudf#14328 and rapidsai/cudf#14329 happened? At least on rapidsai/cudf#14329 the doc-build CI is now passing: https://github.com/rapidsai/cudf/actions/runs/6654117636/job/18083387254?pr=14329

[choldgraf]

I'm not sure that I understand the nature of this issue perfectly, but I'll just note that I think it's not a good idea to raise Sphinx warnings just as a way of giving the user notifications (at least anything in a way that will fail with -W). I know a lot of projects that tell Sphinx to "fail on warning" in order to catch regressions in their docs.

[vyasr]

@drammock I apologize, you're totally right and I missed that it had been added in the interim. It looks like we are seeing the correct behavior when the key is explicitly set. Thanks!

[drammock]

ok, #1542 adds a new config var surface_warnings that can be set to False to suppress all warnings generated in theme code.