Stop exiting early with `--fail-on-warnings`; add `--exception-on-warning` #12743

AA-Turner · 2024-08-07T10:53:04Z

This is sort-of an alternative to #12727 (cc @jbms), but also is something I've wanted to do for a while.

Currently, using --fail-on-warnings gives a fairly hard to understand traceback (as it is traced back from the WarningIsErrorFilter rather than the warning itself) and means that on minor errors that are easily fixed, the entire build must be restarted, perhaps multiple times to fix errors incrementally.

We have for some time reccommended --keep-going, which was added in Sphinx 1.8. This PR removes the "early exit" behaviour and instead exits with a non-zero status if errors were found. The --keep-going option and sphinx.logging.skip_warningiserror() context manager are retained, but become no-ops.

As a result, there are quite a few simplifications we can make.

A

jbms · 2024-08-07T16:23:03Z

My objective with #12727 is to be able to get into a pdb shell right where a warning is generated, because often that is necessary to debug issues. As far as I can tell, even with this PR I'd still need to make the --pdb option install a warning filter that raises an exception, though.

picnixz · 2024-08-07T19:08:22Z

I'll need a laptop for that review so only next week. I don't have a strong opinion on those ideas because I never use sphinx with those options.

If I need to debug I put prints everywhere... (I just hack my own Sphinx or use the dev branch) Otherwise I transform warnings into errors most of the time and have simpler builds (no autosummary, custom autodoc with hacks).

I'll need to think of the different flows and scenarios but I'd like to review it on my laptop. If you're in a hurry, I'd suggest asking @chrisjsewell as well since he has more insight on those options in general I think.

chrisjsewell · 2024-08-07T21:05:15Z

Heya,
I do agree that having to do --fail-on-warnings --keep-going is overly verbose, and I feel even there is a case for this being the default (for all builds I basically use and advise this combination, and also on RTD https://docs.readthedocs.io/en/stable/config-file/v2.html#sphinx-fail-on-warning)

but... it does feel quite the breaking change, maybe moreso given we have only just released 8.0 and this is proposed in essentially 8.1 😅

I feel there is a number of different behaviours people may want here:

ignore all warnings; don't except, don't exit with a non-zero code (although I wouldn't advise)
emit a non-zero exit code if any (unsuppressed) warning is encountered (I think the general case, especially for CI)
except on the first (unsuppressed) warning (mainly for debugging, as desired by @jbms)

I think perhaps there is the case for two flags: --fail-on-warnings (2) and --except-on-warning (3)

Previously, warnings emitted during reading and writing were deferred, which made --pdb ineffective for debugging them. With this change, warnings are no longer deferred when --pdb and --debug-warnings are both specified. Co-authored-by: Jeremy Maitin-Shepard <jbms@google.com>

AA-Turner · 2024-08-09T22:50:45Z

@jbms I've adapted your patch and merged it into this branch. My new proposal is that --pdb --debug-warnings will run the debugger for warnings. Does this meet your requirements?

A

picnixz

I agree with Chris that those changes are breaking changes and I don't think it should be added to 8.x but rather to 9.x.

Apart from what Chris said, there is a category of people that likely want to exit as soon as possible, but without necessarily using pdb. Namely, they want to turn warnings into errors but don't want to debug them programatically. In other words, the current --fail-on-warning behaviour should be kept but we could instead try to make the traceback nicer (though I don't know if it's possible).

Personally, I don't mind keeping the status quo for the current --fail-on-warning/--keep-going while adding --debug-warnings when running pdb. For me those are two different issues.

CHANGES.rst

sphinx/application.py

sphinx/ext/autodoc/__init__.py

sphinx/util/logging.py

AA-Turner · 2024-08-12T13:50:01Z

I disagree that this is a breaking change, as the important behaviour (exit codes) is maintained.

Apart from what Chris said, there is a category of people that likely want to exit as soon as possible, but without necessarily using pdb. Namely, they want to turn warnings into errors but don't want to debug them programatically. In other words, the current --fail-on-warning behaviour should be kept but we could instead try to make the traceback nicer (though I don't know if it's possible).

I'm not really convinced by this -- I think the current behaviour isn't optimal user experience, and we wouldn't design a system like that today. Contrast with a linter like Flake8, MyPy, or Ruff -- they work hard to show as many errors as possible.

Please could you expand on the rationale for keeping the current 'exit early' behaviour? I think that with the new debug warnings option, this is backwards compatible.

A

picnixz

Please could you expand on the rationale for keeping the current 'exit early' behaviour?

FTR, I'm not against this feature (I'm a +0.5). You said:

means that on minor errors that are easily fixed, the entire build must be restarted, perhaps multiple times to fix errors incrementally

Where I disagree is that people might like to do it that way. For instance, it may not necessarily be the case that fixing one warning in the list of warnings would not create more warnings. So fixing one warning at a time could fix multiple warnings, or create new ones, that would then require to be fixed again. Now, I agree that this might not be the best idea as a default (hence my +0.5) but I still think it'd be nice to be able to fix the warnings incrementally without using pdb (which people may not know how to use). If you're not convinced, I don't mind giving up on this since what I suggested could also be considered as a "dirty and non-recommended approach to fix issues".

I disagree that this is a breaking change, as the important behaviour (exit codes) is maintained.

You (half)-convinced me on that matter. What is actually breaking is that --keep-going is now a no-op. I'd suggest adding a deprecation warning in case the option is used (just create an Action class that does nothing except emitting a warning so that we can remove that option later).

sphinx/cmd/build.py

sphinx/util/logging.py

sphinx/application.py

# Conflicts: # CHANGES.rst # tests/test_builders/test_build_warnings.py # tests/test_quickstart.py # tests/test_util/test_util_logging.py

AA-Turner · 2024-08-13T15:21:37Z

I'd suggest adding a deprecation warning in case the option is used (just create an Action class that does nothing except emitting a warning so that we can remove that option later).

I agree, but let's not deprecate for a while. See the comment in the docs for --keep-going.

AA-Turner · 2024-08-13T15:36:34Z

I'm going to merge this -- @jbms please could you confirm if anything further is needed for your goals?

A

chrisjsewell · 2024-08-13T15:42:35Z

@AA-Turner

--debug-warnings is honestly too overcomplicated, there should be an --except-on-warning option, as simple as that.
I would predict, that you will end up having to implement this sooner or later, when a bunch of people complain that this is no longer possible

AA-Turner · 2024-08-13T15:45:08Z

I'm happy to change to --except-on-warning, but I'm still unconvinced on need. Do you have any examples to mind of similar tools that have an option to exit early?

A

chrisjsewell · 2024-08-13T16:06:58Z

Do you have any examples to mind of similar tools that have an option to exit early?

I'm sure I could hunt one down, but really that is not the point; the point is that sphinx has the option now and, with this, it will no longer. Also sphinx is different to something like say ruff, in that it can be a lot slower; taking many minutes rather than milliseconds.

I know of sphinx builds that can even take up to hours to build and so, especially on CI, sometimes people do not want to not waste time running the build to completion, just to see if the CI build fails.

Take for example: https://github.com/numpy/numpy/blob/be296e2c032f7fbf30534fbe40af43c9f6b6f914/.circleci/config.yml#L77,
this currently "fails fast",
perhaps the numpy folks won't be so happy when they realise they now have to wait ~6 minutes on each PR commit, to see if the docs are failing

AA-Turner · 2024-08-13T16:16:26Z

Also sphinx is different to something like say ruff, in that it can be a lot slower; taking many minutes rather than milliseconds.

This is persuasive, I suppose mypy & ruff are in the order of seconds, whereas Sphinx is in the order of minutes. With the update you suggested, we maintain the ability to "fail fast" with --exception-on-warning, so hopefully those wanting this behaviour are now able to more explicitly opt-in to it.

A

larsoner · 2024-08-19T23:57:24Z

FWIW having the ability to set exception_on_warning=True made my life easier to adapt to this change in sphinx-gallery!

AA-Turner added 3 commits August 7, 2024 11:02

Always --keep-going

4c09772

Simplify things

4d7fab8

ruff

af40e77

AA-Turner added the type:enhancement enhance or introduce a new feature label Aug 7, 2024

AA-Turner added this to the 8.x milestone Aug 7, 2024

AA-Turner requested review from jbms and picnixz August 7, 2024 10:53

AA-Turner added 2 commits August 7, 2024 11:55

autodoc_warningiserror

7dbe321

_fail_on_warnings

d14b2e0

AA-Turner changed the title ~~Stop exiting early with --fail-on-warnings~~ Stop exiting early with --fail-on-warnings; add --debug-warnings Aug 9, 2024

DummyApplication

cbabdeb

picnixz reviewed Aug 12, 2024

View reviewed changes

sphinx/cmd/build.py Outdated Show resolved Hide resolved

sphinx/util/logging.py Outdated Show resolved Hide resolved

sphinx/application.py Outdated Show resolved Hide resolved

AA-Turner added 3 commits August 13, 2024 16:10

fixups

227685c

more fixups

57ebb22

Merge branch 'master' into always-keep-going

c8e72f5

# Conflicts: # CHANGES.rst # tests/test_builders/test_build_warnings.py # tests/test_quickstart.py # tests/test_util/test_util_logging.py

AA-Turner added 2 commits August 13, 2024 16:29

Alias skip_warningiserror to nullcontext

ac1b8bb

DummyApplication

d7dac4c

rename to --exception-on-warning

202c907

AA-Turner changed the title ~~Stop exiting early with --fail-on-warnings; add --debug-warnings~~ Stop exiting early with --fail-on-warnings; add --exception-on-warning Aug 13, 2024

AA-Turner merged commit fadb6b1 into sphinx-doc:master Aug 13, 2024
20 checks passed

AA-Turner deleted the always-keep-going branch August 13, 2024 16:13

AA-Turner mentioned this pull request Aug 14, 2024

Allow --pdb option to also debug warnings #12727

Closed

github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2024

AA-Turner modified the milestones: 8.x, 8.1.0 Oct 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop exiting early with `--fail-on-warnings`; add `--exception-on-warning` #12743

Stop exiting early with `--fail-on-warnings`; add `--exception-on-warning` #12743

AA-Turner commented Aug 7, 2024

jbms commented Aug 7, 2024

picnixz commented Aug 7, 2024

chrisjsewell commented Aug 7, 2024 •

edited

Loading

AA-Turner commented Aug 9, 2024

picnixz left a comment

AA-Turner commented Aug 12, 2024

picnixz left a comment

AA-Turner commented Aug 13, 2024

AA-Turner commented Aug 13, 2024

chrisjsewell commented Aug 13, 2024 •

edited

Loading

AA-Turner commented Aug 13, 2024

chrisjsewell commented Aug 13, 2024 •

edited

Loading

AA-Turner commented Aug 13, 2024

larsoner commented Aug 19, 2024

Stop exiting early with --fail-on-warnings; add --exception-on-warning #12743

Stop exiting early with --fail-on-warnings; add --exception-on-warning #12743

Conversation

AA-Turner commented Aug 7, 2024

jbms commented Aug 7, 2024

picnixz commented Aug 7, 2024

chrisjsewell commented Aug 7, 2024 • edited Loading

AA-Turner commented Aug 9, 2024

picnixz left a comment

Choose a reason for hiding this comment

AA-Turner commented Aug 12, 2024

picnixz left a comment

Choose a reason for hiding this comment

AA-Turner commented Aug 13, 2024

AA-Turner commented Aug 13, 2024

chrisjsewell commented Aug 13, 2024 • edited Loading

AA-Turner commented Aug 13, 2024

chrisjsewell commented Aug 13, 2024 • edited Loading

AA-Turner commented Aug 13, 2024

larsoner commented Aug 19, 2024

Stop exiting early with `--fail-on-warnings`; add `--exception-on-warning` #12743

Stop exiting early with `--fail-on-warnings`; add `--exception-on-warning` #12743

chrisjsewell commented Aug 7, 2024 •

edited

Loading

chrisjsewell commented Aug 13, 2024 •

edited

Loading

chrisjsewell commented Aug 13, 2024 •

edited

Loading