CI: Fail doc build on warning #22743
Conversation
Warning about elementwise comparison failing when we indexed a dataframe with boolean dataframe
|
Hello @TomAugspurger! Thanks for updating the PR.
Comment last updated on September 20, 2018 at 20:51 Hours UTC |
| .. toctree:: | ||
| :hidden: | ||
|
|
||
| generated/pandas.api.extensions.ExtensionDtype.na_value |
There was a problem hiding this comment.
No idea on this one. Something with numpydoc / sphinx autodoc. I think autodoc detects that na_value has a "docstring" (comments), but numpydoc doesn't. Not super important.
| mask = g < 0 | ||
| g.loc[mask] = g[~mask].mean() | ||
| return g | ||
| mask = g < 0 |
There was a problem hiding this comment.
Apparently the argument passed to func in transform can be either a Series or DataFrame? This implementation should be robust to either.
| :func:`read_csv` and :func:`read_table`. They both use the same parsing code to | ||
| intelligently convert tabular data into a ``DataFrame`` object. See the | ||
| :ref:`cookbook<cookbook.csv>` for some advanced strategies. | ||
| The workhorse function for reading text files (a.k.a. flat files) is |
There was a problem hiding this comment.
I basically removed all references to read_table not that it's deprecated.
| @@ -312,14 +312,15 @@ All one-dimensional list-likes can be combined in a list-like container (includi | |||
|
|
|||
| s | |||
| u | |||
| s.str.cat([u.values, ['A', 'B', 'C', 'D'], map(str, u.index)], na_rep='-') | |||
| s.str.cat([u.values, | |||
| u.index.astype(str).values], na_rep='-') | |||
There was a problem hiding this comment.
@h-vetinari the list inside a list was causing issues. I've removed it from the examples. It'd be good to write new ones demonstrating that though.
There was a problem hiding this comment.
I guess this was an oversight in #22264 where we deprecated lists within lists.
It'd be good to write new ones demonstrating that though.
New examples?
| natural and functions similarly to :py:func:`itertools.groupby`: | ||
|
|
||
| .. ipython:: python | ||
|
|
||
| resampled = df.resample('H') |
There was a problem hiding this comment.
At this point, df is a DataFrame with columns like ['year', 'day', 'hour'...]. This most have been expecting something different.
|
|
||
| .. ipython:: python | ||
|
|
||
| ts = pd.Timestamp('2014-01-01 09:00') |
There was a problem hiding this comment.
cc @jorisvandenbossche @jbrockmendel if you want to take a look at these to ensure I'm doing this properly (#21564)
| @@ -9541,14 +9548,16 @@ def to_csv(self, path_or_buf=None, sep=",", na_rep='', float_format=None, | |||
| encoding : string, optional | |||
| A string representing the encoding to use in the output file, | |||
| defaults to 'ascii' on Python 2 and 'utf-8' on Python 3. | |||
| compression : {'infer', 'gzip', 'bz2', 'zip', 'xz', None}, | |||
There was a problem hiding this comment.
I couldn't get this line-wrapping to work properly by ending the line with a backslash. Does anyone know the correct syntax? I'm inclided to just get the docs right and # noqa at the end of the line.
| @@ -135,6 +136,12 @@ def _process_single_doc(self, single_doc): | |||
| try: | |||
| obj = pandas # noqa: F821 | |||
| for name in single_doc.split('.'): | |||
| try: | |||
There was a problem hiding this comment.
cc @datapythonista single-page pandas.io.formats.style.Styler wasn't working earlier, since it's not available from the top-level namespace. Do we have tests for this?
Codecov Report
@@ Coverage Diff @@
## master #22743 +/- ##
=======================================
Coverage 92.17% 92.17%
=======================================
Files 169 169
Lines 50739 50739
=======================================
Hits 46767 46767
Misses 3972 3972
Continue to review full report at Codecov.
|
|
https://travis-ci.org/pandas-dev/pandas/jobs/431185566#L5353 I think it's still not failing because Travis is calling |
|
We'll need to either figure out the rpy2 on travis issue, or figure out a way to "whitelist" errors. Maybe this would be a good reason to move the doc build to azure. |
|
I'm going to split off the changes to the docs to fix the warnings. We can followup later with the infrastructure to fail the build. |
|
That sounds good. Did you see that |
|
The problem with `-W` is that it fails the build immediately and stops
building the docs. We would rather have the doc build finish to completion,
but fail CI if any warnings were present.
…On Sun, Dec 16, 2018 at 8:46 PM Marc Garcia ***@***.***> wrote:
That sounds good. Did you see that sphinx-build has an argument -W that
should fail the doc build if warnings are found? I'm working on a PR to
speed up running doc/make.py with --single, will check also if that
option works.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#22743 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIhQZmAHxBaoXDVlgF9BzQgdqHk1Aks5u5wV5gaJpZM4WuRqM>
.
|
|
Will you continue the work to split off the changes in this PR, or will you open new ones and should we close this? I'm not sure if there are many warnings left of the ones you were fixing here. I think many have been fixed when linting the code in the docs, and I've been fixing some too. I agree that ideally we'd like to run the whole doc build and not cancel at the first warning. But given that cancelling is implemented just adding a parameter to the build call, and the complexity needed to run the whole build, I'd prefer to just use |
|
I think that most of the warnings are fixed on master.
I don't think I would support adding a `-W` flag to the build. That was my
first approach, and even locally the feedback loop was just too slow. Doing
it on the CI would be even worse.
I'd prefer the status quo of manually checking for warnings over failing on
the first warning.
Perhaps we could push this upstream to sphinx though.
…On Sun, Dec 30, 2018 at 6:06 PM Marc Garcia ***@***.***> wrote:
Will you continue the work to split off the changes in this PR, or will
you open new ones and should we close this?
I'm not sure if there are many warnings left of the ones you were fixing
here. I think many have been fixed when linting the code in the docs, and
I've been fixing some too.
I agree that ideally we'd like to run the whole doc build and not cancel
at the first warning. But given that cancelling is implemented just adding
a parameter to the build call, and the complexity needed to run the whole
build, I'd prefer to just use -W. In the PRs, we are discarding the
generated docs anyway, and if that makes the build fail, we shouldn't have
any warnings in master. So, in practice, the difference is that we'll just
see the first warning in the CI and not all them. Which I don't think
justifies the extra complexity in the build.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#22743 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIqK9qsBesAU89lH0q6-9Ut-WD6Mmks5u-VULgaJpZM4WuRqM>
.
|
|
Ahh, maybe it's already done? Does the `--keep-going` flag do what we need?
```
bash-4.4$ make html SPHINXOPTS='-W --keep-going'
Running Sphinx v1.8.3
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 1 source files that are out of date
updating environment: 1 added, 0 changed, 0 removed
reading sources... [100%] index
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [100%] index
/private/tmp/sphinx-tst/index.rst:14: WARNING: undefined label: a (if the
link has no caption the label must precede a section header)
/private/tmp/sphinx-tst/index.rst:16: WARNING: undefined label: b (if the
link has no caption the label must precede a section header)
generating indices... genindex
writing additional pages... search
copying static files... done
copying extra files... done
dumping search index in English (code: en) ... done
dumping object inventory... done
build finished with problems, 2 warnings.
make: *** [html] Error 1
```
That would be great.
On Mon, Dec 31, 2018 at 7:38 AM Tom Augspurger <tom.augspurger88@gmail.com>
wrote:
… I think that most of the warnings are fixed on master.
I don't think I would support adding a `-W` flag to the build. That was my
first approach, and even locally the feedback loop was just too slow. Doing
it on the CI would be even worse.
I'd prefer the status quo of manually checking for warnings over failing
on the first warning.
Perhaps we could push this upstream to sphinx though.
On Sun, Dec 30, 2018 at 6:06 PM Marc Garcia ***@***.***>
wrote:
> Will you continue the work to split off the changes in this PR, or will
> you open new ones and should we close this?
>
> I'm not sure if there are many warnings left of the ones you were fixing
> here. I think many have been fixed when linting the code in the docs, and
> I've been fixing some too.
>
> I agree that ideally we'd like to run the whole doc build and not cancel
> at the first warning. But given that cancelling is implemented just adding
> a parameter to the build call, and the complexity needed to run the whole
> build, I'd prefer to just use -W. In the PRs, we are discarding the
> generated docs anyway, and if that makes the build fail, we shouldn't have
> any warnings in master. So, in practice, the difference is that we'll just
> see the first warning in the CI and not all them. Which I don't think
> justifies the extra complexity in the build.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#22743 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIqK9qsBesAU89lH0q6-9Ut-WD6Mmks5u-VULgaJpZM4WuRqM>
> .
>
|
|
Yes, didn't see the option, but it works as expected. :) I had to make couple of changes to make our script behave properly when setting this option, but implemented it in #24523. Do you want to take a look at this PR and see if there is anything you want to commit. Otherwise we can close it. |
|
Closing this, as failing the doc build has been implemented in #24523, and I think all the warnings fixed here have already been fixed somewhere else. Feel free to reopen if there is anything that could be merged from here, and you want to reuse this PR for it. |
|
I don't see those regex. I implemented a regex to find wrong spaces in the directives in #24650. I couldn't find anything in this PR that is not fixed already (at least the parts generating warnings). But as said, feel free to reopen if it's still useful. |
The |
|
oh, I see. I thought you meant a regex over the rst files, not the log. I assume sphinx will return with an exit status != 0 if the process raises those (with the |
I am not sure that is the case. From a quick test, sphinx doesn't seem to regard an IPython warning as a proper sphinx warning (but that might something that could be fixed in IPython). (Although that also relates to our use of |
|
but letting sphinx turn a warning into an error,
I think you meant error into warning (and then into an error)? IIUC our
ideal workflow is
1. We set a new IPython setting `ipython_error_is_warning = True` (maybe
exception_is_warning)
2. An error in an IPython block emits a warning, which sphinx logs, and the
doc build continues
3. Sphinx finishes, sees that there was an IPytyhon warning, and fails the
build
…On Mon, Jan 7, 2019 at 6:51 AM Joris Van den Bossche < ***@***.***> wrote:
I assume sphinx will return with an exit status != 0 if the process raises
those (with the -W option).
I am not sure that is the case. From a quick test, sphinx doesn't seem to
regard an IPython warning as a proper sphinx warning (but that might
something that could be fixed in IPython).
(Although that also relates to our use of ipython_warning_is_error = False,
so erroring directly is possible (but we choose not to do that, as it stops
the build), but letting sphinx turn a warning into an error, not sure about
that)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#22743 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIqDSnFZa3UCPp3hlWPd7_9v_XgLLks5vA0LngaJpZM4WuRqM>
.
|
Well, whether IPython directly emits a warning, or we have a way to turn their error in a warning, that doesn't matter too much to me :-) As long as the end result is that there is a warning, that can be turned into an exit code error with sphinx. I would say that ideally, IPython uses the proper sphinx warning system and issues a sphinx warning instead of printing their warning to stdout, so that the sphinx warning machinery works with them properly? |
|
Yep. I'll raise an issue upstream. |
(continuing a theme)
xref #21564
We print the doc build output to a file and parse that file for errors. This is preferable to Sphinx'x built-in ability to raise on errors, since we will print warnings for all the errors present in a PR, rather than just the first (and immediately exiting the build).