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

[ci] make check-docs job compatible with rstcheck 6.x #5388

Merged
merged 4 commits into from
Jul 28, 2022
Merged

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Jul 25, 2022

The check-docs CI job has started failing recently, with the following error (click here for example failing build).

Error: No such option: --report Did you mean --report-level?

I believe this is because of some breaking changes included in rstcheck 6.0.0. Described in these issues:

According to https://anaconda.org/conda-forge/rstcheck, rstcheck v6.0.0 was just published to conda-forge about 3 days ago.

Screen Shot 2022-07-25 at 6 15 25 PM

This PR proposes a fix to make LightGBM's CI compatible with that version.

It also proposes putting in a floor of rstcheck>=6.0.0 so that we'll get a loud, obvious error if a surprising environment solve leads to rstcheck being downgraded in the future.

It also makes the following changes to resolve errors raised by the new rstcheck version:

  • ignores autosummary directive in checks
  • adds an explicit language to all code blocks in .rst files which didn't have one

References

Started looking into this after @svotaw pointed it out in #5299 (comment).

I hadn't noticed it since CI has been broken anyway for the last week because of #5387 . Once #5387, we'll need to merge this next to unblock CI again.

@jameslamb
Copy link
Collaborator Author

This new version of rstcheck is raising some additional errors.

CRITICAL:rstcheck_core.checker:An `AttributeError` error occured. This is most propably due to a code block directive (code/code-block/sourcecode) without a specified language. This may result in a false negative for source: 'GPU-Windows.rst'. See https://rstcheck-core.readthedocs.io/en/latest/faq/#code-blocks-without-language-sphinx for more information.
CRITICAL:rstcheck_core.checker:An `AttributeError` error occured. This is most propably due to a code block directive (code/code-block/sourcecode) without a specified language. This may result in a false negative for source: 'Installation-Guide.rst'. See https://rstcheck-core.readthedocs.io/en/latest/faq/#code-blocks-without-language-sphinx for more information.
CRITICAL:rstcheck_core.checker:An `AttributeError` error occured. This is most propably due to a code block directive (code/code-block/sourcecode) without a specified language. This may result in a false negative for source: 'Parallel-Learning-Guide.rst'. See https://rstcheck-core.readthedocs.io/en/latest/faq/#code-blocks-without-language-sphinx for more information.
CRITICAL:rstcheck_core.checker:An `AttributeError` error occured. This is most propably due to a code block directive (code/code-block/sourcecode) without a specified language. This may result in a false negative for source: 'Experiments.rst'. See https://rstcheck-core.readthedocs.io/en/latest/faq/#code-blocks-without-language-sphinx for more information.
Python-API.rst:9: (ERROR/3) Unknown directive type "autosummary".
Python-API.rst:20: (ERROR/3) Unknown directive type "autosummary".
Python-API.rst:29: (ERROR/3) Unknown directive type "autosummary".
Python-API.rst:42: (ERROR/3) Unknown directive type "autosummary".
Python-API.rst:52: (ERROR/3) Unknown directive type "autosummary".
Python-API.rst:63: (ERROR/3) Unknown directive type "autosummary".
Python-API.rst:75: (ERROR/3) Unknown directive type "autosummary".

I'm going to move this back to "draft". If I can't solve these today, I'll change this PR to putting a ceiling on rstcheck to unblock the rest of CI, and open a separate issue for resolving these errors.

@jameslamb jameslamb marked this pull request as draft July 26, 2022 14:46
@jameslamb jameslamb changed the title [ci] make check-docs job compatible with rstcheck 5.x WIP: [ci] make check-docs job compatible with rstcheck 5.x Jul 26, 2022
@jameslamb jameslamb marked this pull request as ready for review July 26, 2022 15:00
@jameslamb jameslamb changed the title WIP: [ci] make check-docs job compatible with rstcheck 5.x [ci] make check-docs job compatible with rstcheck 5.x Jul 26, 2022
@jameslamb
Copy link
Collaborator Author

Ok check-docs task is passing, so I believe this is ready for review.

@StrikerRUS could you please enable this branch at readthedocs so we can check?

(or add me as an admin there so I don't have to bother you 😛)

@jameslamb
Copy link
Collaborator Author

😭 sadly, more CI failures that weren't noticed before because of #5387 . We are getting really unlucky this week 😭

All cuda jobs and several Linux jobs are failing with the following.

FAILED ../tests/python_package_test/test_dask.py::test_machines_should_be_used_if_provided[binary-classification]
FAILED ../tests/python_package_test/test_dask.py::test_machines_should_be_used_if_provided[multiclass-classification]
FAILED ../tests/python_package_test/test_dask.py::test_machines_should_be_used_if_provided[regression]
FAILED ../tests/python_package_test/test_dask.py::test_machines_should_be_used_if_provided[ranking]
= 4 failed, 700 passed, 10 skipped, 2 xfailed, 395 warnings in 655.51s (0:10:55) =

(example build link).

client.restart() calls in that test are resulting in the following:

raise TimeoutError(f"{len(bad_nannies)}/{len(nannies)} nanny worker(s) did not shut down within {timeout}s")
E asyncio.exceptions.TimeoutError: 1/2 nanny worker(s) did not shut down within 120s

traceback (click me)
/root/miniforge/envs/test-env/lib/python3.9/site-packages/distributed/client.py:3360: in restart
    return self.sync(
/root/miniforge/envs/test-env/lib/python3.9/site-packages/distributed/utils.py:338: in sync
    return sync(
/root/miniforge/envs/test-env/lib/python3.9/site-packages/distributed/utils.py:405: in sync
    raise exc.with_traceback(tb)
/root/miniforge/envs/test-env/lib/python3.9/site-packages/distributed/utils.py:378: in f
    result = yield future
/root/miniforge/envs/test-env/lib/python3.9/site-packages/tornado/gen.py:762: in run
    value = future.result()
/root/miniforge/envs/test-env/lib/python3.9/site-packages/distributed/client.py:3329: in _restart
    await self.scheduler.restart(timeout=timeout, wait_for_workers=wait_for_workers)
/root/miniforge/envs/test-env/lib/python3.9/site-packages/distributed/core.py:1153: in send_recv_from_rpc
    return await send_recv(comm=comm, op=key, **kwargs)
/root/miniforge/envs/test-env/lib/python3.9/site-packages/distributed/core.py:943: in send_recv
    raise exc.with_traceback(tb)
/root/miniforge/envs/test-env/lib/python3.9/site-packages/distributed/core.py:769: in _handle_comm
    result = await result
/root/miniforge/envs/test-env/lib/python3.9/site-packages/distributed/utils.py:778: in wrapper
    return await func(*args, **kwargs)

It looks like those jobs are getting dask and distributed 2022.7.1

    dask-2022.7.1              |     pyhd8ed1ab_0           5 KB  conda-forge
    dask-core-2022.7.1         |     pyhd8ed1ab_0         840 KB  conda-forge
    dbus-1.13.6                |       h5008d03_3         604 KB  conda-forge
    distributed-2022.7.1       |     pyhd8ed1ab_0         735 KB  conda-forge

which hit conda-forge 3 days ago.

Screen Shot 2022-07-26 at 1 34 56 PM

I'll just pushed 8e4d3a3 pinning to the previous dask / distributed release to see if that helps.

I'll try separately investigating this new issue later tonight or maybe tomorrow (whenever I have time).

@jameslamb
Copy link
Collaborator Author

jameslamb commented Jul 26, 2022

Created #5390 to track the Dask work.

Pushed b6ab42f with an approach similar to #4770 to unblock this PR. Hopefully we can merge this to unblock the rest of CI, and I can work on #5390 separately.

@shiyu1994
Copy link
Collaborator

@jameslamb Thanks for working on this. Seems that we can now successfully pass the doc checks. Can we now merge this PR?

@jameslamb
Copy link
Collaborator Author

Because this changed the documentation, before we merge I'd like to confirm that the docs built correctly at readthedocs.

Unfortunately, I'm not an admin there and don't have permissions to trigger a build for this branch. Right now it looks like only @guolinke @henry0312 and @StrikerRUS are admins on the project (https://readthedocs.org/projects/lightgbm/).

I'd like to wait a few more hours to see if one of them responds soon and either enables a build for this branch of gives me permissions to do so. If not, then I think in a few hours we should just merge this to block CI for other PRs, with the understanding that if the docs break we'll need to fix them.

@shiyu1994
Copy link
Collaborator

I think readthedocs only builds the documentation when we push to the master branch.

@jameslamb
Copy link
Collaborator Author

jameslamb commented Jul 28, 2022

I think readthedocs only builds the documentation when we push to the master branch.

It has been standard practice in this project for several years to manually, temporarily enable readthedocs builds for PR branches affecting documentation.

For example: #5381 (comment)

@jameslamb
Copy link
Collaborator Author

I'll at least try right now with the containerized process documented in https://github.com/microsoft/LightGBM/tree/f94050a4cc94909e10a0064baff11cec795eb250/docs#docker.

If the docs build successfully and the files I've changed here look ok, then I think we can merge this, to keep making progress on other PRs.

I'll post an update shortly.

@jameslamb
Copy link
Collaborator Author

In the dockerized setup, docs look ok! Checked the places that this PR changed.

I'm going to merge this, and try to trigger builds of other approved PRs (slowly, so they don't all get stuck due to a backup of CUDA CI jobs).

Experiments.rst

image

GPU-Windows.rst

image

image

Installation-Guide.rst

image

Parallel-Learning-Guide.rst

image

@jameslamb jameslamb changed the title [ci] make check-docs job compatible with rstcheck 5.x [ci] make check-docs job compatible with rstcheck 6.x Jul 28, 2022
@jameslamb jameslamb added doc and removed maintenance labels Jul 28, 2022
@jameslamb jameslamb merged commit 8d12dca into master Jul 28, 2022
@jameslamb jameslamb deleted the ci/rstcheck branch July 28, 2022 04:23
@jameslamb
Copy link
Collaborator Author

After merging this, the readthedocs build succeeded on master: https://readthedocs.org/projects/lightgbm/builds/17559437/

And docs at https://lightgbm.readthedocs.io/en/latest/Installation-Guide.html look ok to me.

@westurner
Copy link

westurner commented Jul 28, 2022 via email

@StrikerRUS
Copy link
Collaborator

@jameslamb

or add me as an admin there

Seems that I'm able to do this! Please share your username (or register account if you don't have one) at RTD.

@jameslamb
Copy link
Collaborator Author

Thanks @StrikerRUS !

My username on RTD is jameslamb.

@StrikerRUS
Copy link
Collaborator

@jameslamb
Done!

image

@jameslamb
Copy link
Collaborator Author

Thanks so much!

@jameslamb jameslamb mentioned this pull request Oct 7, 2022
40 tasks
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants