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] [docs] use miniforge for readthedocs builds (fixes #4954) #4957

Merged
merged 23 commits into from
Feb 19, 2022

Conversation

jameslamb
Copy link
Collaborator

Fixes #4954.
Contributes to #4948.
Contributes to #4859.

This PR proposes updating LightGBM's readthedocs configuration to tell RTD to use mamba to manage dependencies. See the description of #4954 for relevant links to RTD documentation describing these new config options.

Notes for Reviewers

This PR updates the "check-docs" CI job to use conda. I'm hoping this can be merged before #4953, and then #4953 could switch that CI check to use mamba so LightGBM's CI will more closely match what happens on master RTD builds.

@StrikerRUS could you please temporarily enable builds for this branch on RTD so we can test the new configuration?

@StrikerRUS
Copy link
Collaborator

could you please temporarily enable builds for this branch on RTD so we can test the new configuration?

Sure, done!
https://readthedocs.org/projects/lightgbm/builds/15811372/

docs/README.rst Outdated Show resolved Hide resolved
@jameslamb
Copy link
Collaborator Author

Sure, done!
https://readthedocs.org/projects/lightgbm/builds/15811372/

Thanks for that! I'm really happy to see that the build succeeded, docs look correct (I checked Python, C API, and the R docs), and that the build didn't take longer than usual.

@jameslamb jameslamb requested a review from StrikerRUS January 19, 2022 00:50
@StrikerRUS
Copy link
Collaborator

Hmm... check-docs job failed with

Running Sphinx v4.4.0
making output directory... done
                                                                                             
Exception occurred:
  File "/home/runner/miniconda/envs/docs-env/lib/python3.9/site-packages/sphinx/theming.py", line 207, in load_external_theme
    theme_entry_points = entry_points(group='sphinx.html_themes')
TypeError: entry_points() got an unexpected keyword argument 'group'
The full traceback has been saved in /tmp/sphinx-err-bcgclosn.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
make: *** [Makefile:20: html] Error 2

https://github.com/microsoft/LightGBM/runs/4863199597?check_suite_focus=true

docs/build-docs.sh Outdated Show resolved Hide resolved
docs/build-docs.sh Outdated Show resolved Hide resolved
docs/README.rst Outdated Show resolved Hide resolved
docs/README.rst Outdated Show resolved Hide resolved
Co-authored-by: Nikita Titov <[email protected]>
@jameslamb
Copy link
Collaborator Author

TypeError: entry_points() got an unexpected keyword argument 'group'

I think I understand what's happening.

I've found reports claiming that this error is about a difference in importlib-metadata versions.

importlib.metadata (note the .) has been a part of the Python standard library since Python 3.8 (https://docs.python.org/3/library/importlib.metadata.html#entry-points).

importlib-metadata (note the -) is an external library providing importlib.metadata's functionality. From PyPI (link):

This package supplies third-party access to the functionality of importlib.metadata including improvements added to subsequent Python versions.

New features are introduced in this third-party library and later merged into CPython.

The conda-forge feedstock for importlib-metadata was archived on conda-forge in 2019 (https://github.com/conda-forge/importlib-metadata-feedstock).

It looks like sphinx will first try to use external package importlib_metadata if available, and otherwise will use the standardlib package.

https://github.com/sphinx-doc/sphinx/blob/9a42e42a401444dc833261409fb55a5067409b8e/sphinx/theming.py#L19-L22

try:  # Python < 3.10 (backport)
    from importlib_metadata import entry_points
except ImportError:
    from importlib.metadata import entry_points

I suspect that the issue here is related to mixing conda channels. Notice in https://github.com/microsoft/LightGBM/runs/4863199597?check_suite_focus=true that conda create is getting packages from the anaconda defaults channels, and then conda install is getting conda-forge channels.

python             pkgs/main/linux-64::python-3.9.5-h12debd9_4
readline           pkgs/main/linux-64::readline-8.1-h27cfd23_0
requests           pkgs/main/noarch::requests-2.25.1-pyhd3eb1b0_0
...
...
...
doxygen            conda-forge/linux-64::doxygen-1.9.2-hb166930_0
rstcheck           conda-forge/noarch::rstcheck-3.3.1-pyh9f0ad1d_0

I think that specifying nodefaults in env.yml is either not working as the documentation says it should or I've misunderstood that documentation (docs link).

You can exclude the default channels by adding nodefaults to the channels list.

I tried that approach because conda env create --file does not accept flags like --channel. I'm going to try pushing a commit with a different approach. Note that this issue should go away, and this code should be simplified, once we're using mamba[forge].

@jameslamb
Copy link
Collaborator Author

I haven't been able to figure out the right combination of conda commands to fix this issue.

Since the builds are working correctly on RTD (build link) and in Docker locally (both using mamba[forge]), I'm confident that just using mamba[forge] will resolve these issues.

So I'd like to request changing the order of merging PRs I'd originally suggested in this PR's description. @StrikerRUS could we merge #4953 before this one? Then I'll update this one to use mamba[forge].

@StrikerRUS
Copy link
Collaborator

So I'd like to request changing the order of merging PRs I'd originally suggested in this PR's description. @StrikerRUS could we merge #4953 before this one?

Sure!

@jameslamb
Copy link
Collaborator Author

Now that #4953 has been merged, I'll work on updating this.

https://docs.readthedocs.io/en/stable/config-file/v2.html#build-tools-python says it's possible to set python.version: miniconda3-4.7. I'll switch from mamba to that, based on #4953 (comment) and #4953 (comment)

@jameslamb
Copy link
Collaborator Author

Ok I think this is ready for review!

  1. checks-docs build succeeded (build link)
    • logs show all packages are being installed only from conda-forge
  2. RTD build succeeded (build link)
  3. I tested the docker run command introduced in docs/README.rst in this PR on my Mac tonight. It ran successfully and the docs rendered locally look correct.

@StrikerRUS StrikerRUS changed the title [ci] [docs] use mamba for readthedocs builds (fixes #4954) [ci] [docs] use miniforge for readthedocs builds (fixes #4954) Feb 16, 2022
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Thank you so much for working on this! Please consider checking my fresh comments below.

.ci/test.sh Outdated Show resolved Hide resolved
docs/README.rst Outdated Show resolved Hide resolved
docs/README.rst Outdated Show resolved Hide resolved
docs/README.rst Outdated Show resolved Hide resolved
docs/README.rst Outdated Show resolved Hide resolved
docs/README.rst Outdated Show resolved Hide resolved
docs/build-docs.sh Outdated Show resolved Hide resolved
docs/build-docs.sh Show resolved Hide resolved
Comment on lines +8 to +15
- r-base=4.1.2
- r-data.table=1.14.2
- r-jsonlite=1.7.2
- r-knitr=1.37
- r-matrix=1.4_0
- r-pkgdown=1.6.1
- r-rmarkdown=2.11
- r-roxygen2=7.1.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't want to pin R package versions anymore?.. #2176 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The comment you've linked refers to a commit, 014bee1, that changed the list of package from "not pinned at all" to "pin to aa specific BUILD of a specific VERSION".

This PR's changes are still pinning them to specific package VERSIONS, just not to individual BUILDS. Sometimes new builds of the same package version are pushed to conda channels, to fix issues like, for example, mistakes in metadata about declared dependencies. We should want to get such fixes automatically, as they make it more likely that the environment will solve correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment you've linked refers to a commit, 014bee1, that changed the list of package from "not pinned at all" to "pin to aa specific BUILD of a specific VERSION".

More relevant past discussion: #2725 (comment).

OK, let's not pin build numbers for now. But if RTD starts to fail, we know what to try first.

@jameslamb jameslamb requested a review from StrikerRUS February 17, 2022 02:40
docs/README.rst Outdated Show resolved Hide resolved
docs/README.rst Outdated Show resolved Hide resolved
Comment on lines +8 to +15
- r-base=4.1.2
- r-data.table=1.14.2
- r-jsonlite=1.7.2
- r-knitr=1.37
- r-matrix=1.4_0
- r-pkgdown=1.6.1
- r-rmarkdown=2.11
- r-roxygen2=7.1.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment you've linked refers to a commit, 014bee1, that changed the list of package from "not pinned at all" to "pin to aa specific BUILD of a specific VERSION".

More relevant past discussion: #2725 (comment).

OK, let's not pin build numbers for now. But if RTD starts to fail, we know what to try first.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Just two minor suggestions below and I think this PR can be merged.

@StrikerRUS StrikerRUS merged commit 2f27d4b into master Feb 19, 2022
@StrikerRUS StrikerRUS deleted the fix/rtd-configuration branch February 19, 2022 03:30
@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 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ci] [docs] readthedocs configuration options are deprecated
2 participants