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.

Sign up for GitHub

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

Jump to bottom

[doc] Reference the R doc in sphinx document site. #11166

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from
Draft

[doc] Reference the R doc in sphinx document site. #11166

wants to merge 22 commits into from

Conversation

trivialfis
Copy link
Member

@trivialfis trivialfis commented Jan 14, 2025 •
edited
Loading

todos

  • Registry tag
  • Fix pkgdown error due to missing xgboost.
  • Check release branch.
  • Ensure consistency of documents in master and release branches.

@trivialfis
Copy link
Member Author

trivialfis commented Jan 14, 2025 •
edited
Loading

The integration is quite primitive. We can expand the integration after #11123 is merged if needed.

Thank you @jameslamb for sharing the use of pkgdown, cool project ;-)

Spark and LightGBM both take a similar approach for document integration with additional customization.

@trivialfis
Copy link
Member Author

seems hung, will check again after the cancel is successful. https://app.readthedocs.org/projects/xgboost/builds/26846124/

@trivialfis trivialfis mentioned this pull request Jan 14, 2025
Merged
@trivialfis
Copy link
Member Author

readthedocs/readthedocs.org#11928

@trivialfis
Copy link
Member Author

I need some help from @hcho3 after he is back. We might want to cache the R pkgdown result in an S3 bucket, similar to the JVM documents. Building a whole chain of pkgdown dependencies on the RTD worker is not ideal.

david-cortes
david-cortes reviewed Jan 15, 2025
View reviewed changes
doc/R-package/Makefile Outdated Show resolved Hide resolved
doc/R-package/index.rst Outdated Show resolved Hide resolved
doc/R-package/index.rst Show resolved Hide resolved
@trivialfis
Build R docs with pkgdown.
4d08c07 
install dir.

Restore.

system dependencies.

Update ubuntu version.

work on script.
@trivialfis
use r image. [skip ci]
dbfa528
@trivialfis
Copy link
Member Author

still working in progress, need to upstream a docker image to the ops repo.

@trivialfis
Copy link
Member Author

dmlc/xgboost-devops#6

trivialfis
trivialfis commented Jan 16, 2025
View reviewed changes
doc/conf.py
@@ -35,7 +37,35 @@
release = xgboost.__version__


def run_doxygen():
# Witchcraft alert:
Copy link
Member Author

@trivialfis trivialfis Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

took me a while to figure out how it works and that the existing code was broken after the CI revamp. Would be great if we could use a package manager like conda to handle R packages instead of spawning out a new CI workflow. Suggestions are welcome.

@trivialfis trivialfis marked this pull request as ready for review January 16, 2025 19:12
@trivialfis
Copy link
Member Author

trivialfis commented Jan 16, 2025 •
edited
Loading

@david-cortes Please help review the output document: https://xgboost--11166.org.readthedocs.build/en/11166/ when you are available. (many like ## Error: object 'model' not found still needs to be fixed)

cc @hcho3 @jameslamb for CI changes, related: dmlc/xgboost-devops#6

I spawned a new CI workflow for building R documents with pkgdown, along with some fixes to the existing jvm doc fetcher.

CUDA failures are not related, not sure what's updated there. Please ignore them for now.

trivialfis
trivialfis commented Jan 16, 2025
View reviewed changes
doc/requirements.txt
@@ -10,8 +10,8 @@ numpy
scipy
myst-parser
ray[train]
xgboost_ray
Copy link
Member Author

Choose a reason for hiding this comment

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

this fetches an old version of xgboost from pypi.

trivialfis
trivialfis commented Jan 16, 2025
View reviewed changes
ops/pipeline/build-r-docs-impl.sh

cd R-package
MAKEFLAGS=-j$(nproc) Rscript ./tests/helper_scripts/install_deps.R
# Some examples are failing
Copy link
Member Author

@trivialfis trivialfis Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

cc @david-cortes The error message from pkgdown is not particularly helpful for debugging example failures. I ran the R-package/tests/helper_scripts/run-example.R, and some examples are failing. I only managed to eliminate some of the failures by checking various dependencies (a lot). Still need some work to fix the remaining.

trivialfis
trivialfis commented Jan 16, 2025
View reviewed changes
doc/conf.py

if branch == "master":
res = subprocess.run(["git", "rev-parse", "master"], stdout=subprocess.PIPE)
else:
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need to check release branch?

trivialfis
trivialfis commented Jan 16, 2025
View reviewed changes
ops/pipeline/build-r-docs.sh Outdated

source ops/pipeline/get-docker-registry-details.sh

IMAGE_URI=${DOCKER_REGISTRY_URL}/xgb-ci.cpu_build_r_doc:PR-6
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to change this back to main.

@trivialfis trivialfis marked this pull request as draft January 16, 2025 19:37
@david-cortes
Copy link
Contributor

@david-cortes Please help review the output document: https://xgboost--11166.org.readthedocs.build/en/11166/ when you are available. (many like ## Error: object 'model' not found still needs to be fixed)

cc @hcho3 @jameslamb for CI changes, related: dmlc/xgboost-devops#6

I spawned a new CI workflow for building R documents with pkgdown, along with some fixes to the existing jvm doc fetcher.

CUDA failures are not related, not sure what's updated there. Please ignore them for now.

I think the core failure is:

Error in library(xgboost): there is no package called 'xgboost'

Since the vignette is not being built through R CMD, it probably needs to install the generated artifact first, which should solve most of the errors. Either that, or load it through devtools::load_all() in the same Rscript call that builds the vignettes.

Also, looks like the TOC requires at least one deeper level:
image

Currently appears to be limited at two. Perhaps something like this would do:

.. toctree::
  :maxdepth: 3

Some suggestions for the pkgdown site:

  • It's not obvious that one needs to click "Reference" to get to function documentations. Perhaps it could have it as a more obvious link in the description of the landing page, like it does for the sphinx links:
    image
  • Title "XGBoost R Package for Scalable GBM" doesn't quite give the impression that this is the documentation package for the R package. Perhaps it could use a more obvious title like "XGBoost R Package Documentation".

@david-cortes
Copy link
Contributor

Could also have a warning that the version switcher only has that R documentation package starting at 3.0 - clicking other versions leads to errors:
image

@david-cortes
Copy link
Contributor

These links point to "latest" version for some reason, even though the version being shown has the PR number:
image

@david-cortes
Copy link
Contributor

Looks like the pkgdown site is not rendering R equations correctly - could it be missing some plugin?
image

This is how it looks like in the in-package documentation in RStudio:
image

@trivialfis
Copy link
Member Author

trivialfis commented Jan 16, 2025 •
edited
Loading

Also, looks like the TOC requires at least one deeper level:

Hmm, not sure. I intentionally kept the navigation bar succinct.

Title "XGBoost R Package for Scalable GBM" doesn't quite give the impression

The landing page for the pkgdown site is from the R package README file i believe. We can revise that file.

Could also have a warning that the version switcher only has that R documentation package starting at 3.0

We add documents all the time, I think it's ok to not have this warning. Otherwise we need to annotate many more new docs.

Since the vignette is not being built through R CMD

Yes, I need to add a installation call before the knitr call. Will do it tomorrow. Great to have some initial feedback.

Looks like the pkgdown site is not rendering R equations correctly - could it be missing some plugin?

Good catch, need to investigate.

These links point to "latest" version for some reason, even though the version being shown has the PR number:

It's the read me file I think? Need to double check, it's hard linked, will change it back to stable if possible.

@david-cortes
Copy link
Contributor

It's the read me file I think? Need to double check, it's hard linked, will change it back to stable if possible.

If it needs to be hard-coded, guess "latest" is a better choice. But other internal links elsewhere point to the same version that one is currently browsing.

@david-cortes
Copy link
Contributor

I intentionally kept the navigation bar succinct.

I think it'd be helpful to have more levels. Especially for the python package docs.

@trivialfis
Copy link
Member Author

Found a related issue for the tex rendering r-lib/pkgdown#2704

Some of these R dependencies have system dependencies

System dependencies are handled by the docker image.

But other internal links elsewhere point to the same version that one is currently browsing.

Will double check.

@trivialfis
Copy link
Member Author

  • The tex rendering is fixed, albeit not pretty. We need a fix in pkgdown instead.
  • Changed the title in the R-package/README.md, and did a small cleanup there as well. User documents are linked to stable, while development-related items are linked to the latest.
  • vignettes are fixed by installing xgboost.
  • API link now points to the reference page instead of the landing page.
  • Grouped the functions in reference page.

I will leave other documentation modifications for future PRs and focus on getting the page to build.

@trivialfis
Copy link
Member Author

@hcho3 Do you think it's possible to use a github webhook to trigger readthedocs build, preferably called inside a github action?

Currently, the JVM docs (and R docs in this PR) are built by github actions and downloaded by the read the docs build worker. However, there's no existing mechanism to synchronize the two services. As a result, it's necessary to manually trigger a rebuild in RTD to render the latest document.

related:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-cortes david-cortes david-cortes left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@trivialfis @david-cortes