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

[docs] [R-package] use CRAN-style builds when building pkgdown site #4513

Merged
merged 5 commits into from
Aug 14, 2021

Conversation

jameslamb
Copy link
Collaborator

This PR proposes using CRAN-style builds of the R package on readthedocs (RTD).

Advantages of this approach over CMake builds:

  • (maybe) faster RTD builds, since cmake does not need to be installed
  • easier updates of the conda R dependencies for RTD builds
  • reduced risk of failures when the RTD images change (since the CRAN package is optimized for portability)
  • another step towards publishing vignettes on the {pkgdown} site (contributes to [R-package] Rewrite R demos, replace with vignettes #1944)

@jameslamb jameslamb added the doc label Aug 11, 2021
@jameslamb
Copy link
Collaborator Author

@StrikerRUS can you please re-enable my docs/jlamb branch at readthedocs? I see it isn't being built: https://readthedocs.org/projects/lightgbm/builds/

image

@StrikerRUS
Copy link
Collaborator

@jameslamb

Sure, done! Just a note: we need to keep RTD versions clean (test something and then remove dev branch) because we've started to provide versions for previous releases (#4236). Excess branches might confuse users there.

@jameslamb
Copy link
Collaborator Author

we need to keep RTD versions clean (test something and then remove dev branch)

Thanks! Yep, I understand. As long as I do not have permissions on RTD to do this myself, I'll have to ask you to add and remove a dev branch when I want to test docs.

@jameslamb
Copy link
Collaborator Author

I see that the most recent build failed with an error like this.

-- Building favicons -----------------------------------------------------------
Building favicons with realfavicongenerator.net...
Error : API request failed.
...
...
...
Error: <callr_status_error: callr subprocess failed: API request failed.>
-->
<callr_remote_error in NULL:
 API request failed.>
 in process 1834 

 Stack trace:

 Process 1819:
 1. pkgdown::build_site(lazy = FALSE, install = FALSE, devel = FALSE,  ...
 2. pkgdown:::build_site_external(pkg = pkg, examples = examples,  ...
 3. callr::r(function(..., crayon_enabled, crayon_colors, pkgdown_internet) { ...
 4. callr:::get_result(output = out, options)
 5. throw(newerr, parent = remerr[[2]])

 x callr subprocess failed: API request failed. 

 Process 1834:
 17. (function (..., crayon_enabled, crayon_colors, pkgdown_internet)  ...
 18. pkgdown::build_site(...)
 19. pkgdown:::build_site_local(pkg = pkg, examples = examples, run_dont_run = r ...
 20. pkgdown:::init_site(pkg)
 21. pkgdown:::build_favicons(pkg)
 22. base:::stop("API request failed.", call. = FALSE)
 23. base:::.handleSimpleError(function (e)  ...
 24. h(simpleError(msg, call))

 x API request failed. 

Execution halted
)

https://readthedocs.org/projects/lightgbm/builds/14472426/

That seems unrelated to the changes in this PR. I'm going to try pushing an empty commit to re-trigger the build.

@jameslamb
Copy link
Collaborator Author

ahhh ok actually, the issue about favicons WAS related to the changes here. I was able to reproduce it locally.

The CRAN-style package doesn't contain the contents of R-package/pkgdown (https://github.com/microsoft/LightGBM/tree/926526c838196a7497a85b6b8cf07657a88b69e6/R-package/pkgdown).

That includes the pkgdown/favicons directory, which was created by pkgdown::build_favicons() (mentioned in #3327 (comment)).

I just pushed 5138946, which I think should fix it.

@jameslamb
Copy link
Collaborator Author

I just pushed 5138946, which I think should fix it.

Ok yep, that worked! The RTD build is passing (https://readthedocs.org/projects/lightgbm/builds/14472810/), and the R documentation looks correct to me (https://lightgbm.readthedocs.io/en/docs-jlamb/R/index.html).

I believe this is ready for review.

@jameslamb jameslamb marked this pull request as ready for review August 14, 2021 04:20
@jameslamb jameslamb requested a review from StrikerRUS as a code owner August 14, 2021 04:20
@jameslamb jameslamb requested review from Laurae2 and shiyu1994 August 14, 2021 04:20
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.

LGTM!
I like the advantages you've listed above 🙂 .

As long as I do not have permissions on RTD to do this myself, I'll have to ask you to add and remove a dev branch when I want to test docs.

I can remove a dev branch by default after merging a PR associated with that branch.

@StrikerRUS StrikerRUS merged commit 3c781ba into master Aug 14, 2021
@StrikerRUS StrikerRUS deleted the docs/jlamb branch August 14, 2021 12:39
@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants