-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[R-package] [docs] add intro vignette (#3946) #4775
Conversation
RTD builds are enabled for this branch: https://readthedocs.org/projects/lightgbm/builds/15200979/. |
/gha run r-solaris Workflow Solaris CRAN check has been triggered! 🚀 solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.3.1.99.tar.gz-08c15b9804194123b4d05bd2d9a260f4 |
/gha run r-valgrind Workflow R valgrind tests has been triggered! 🚀 Status: success ✔️. |
Yeah, seems vignettes were built correctly, that's great! However, I have one concern. That build with only one tiny vignette took ~620 sec compared to less than 500 sec for |
I think that the particular vignette added in this PR probably added very little to that timing, and the most of the increase is attributable to the fact that LightGBM has to now be compiled twice (once each on these two lines). Lines 283 to 284 in aafedd8
If I'm right about that, then it means that adding additional vignettes won't add a lot of time to those builds. I'll test that theory by pushing a test commit here that introduces a copy of Other strategies we could use if the amount of time to build vignettes becomes problematic in the future:
|
I just pushed 40fb2e2 which adds 10 identical copies of https://lightgbm.readthedocs.io/en/dev-vignettes/R/articles/index.html The RTD build took 655s (https://readthedocs.org/projects/lightgbm/builds/15206531/), only about 30s longer than the build with a single vignette (https://readthedocs.org/projects/lightgbm/builds/15203195/). So I think this is strong evidence of what I mentioned in #4775 (comment), that most of the added build time in RTD builds from this PR is from re-compiling LightGBM. As more vignettes are added in #1944 we should consider the strategies for limiting runtime that I mentioned in #4775 (comment), but I think there's a good amount of room to add new vignettes without risking hitting that RTD 900-second limit. |
This reverts commit 40fb2e2.
@jameslamb Thanks for your investigation! I agree we may add some number of new vignettes without any worries. |
/gha run r-solaris Workflow Solaris CRAN check has been triggered! 🚀 solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.3.1.99.tar.gz-b9d6b89a01a1479c99c16fa742ac7604 |
/gha run r-valgrind Workflow R valgrind tests has been triggered! 🚀 Status: success ✔️. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really happy this PR allows to start writing cool vignettes! 🎉
The only disappointing thing is that tar trick.
Please check some my minor suggestions below:
Co-authored-by: Nikita Titov <[email protected]>
agree 😢 Following https://www.r-project.org/bugs.html, I've request access to R's issue tracker. If that's approved, I'll open a bug report (and maybe even attempt a patch), based on the information in the third item mentioned at #3946 (review). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks @StrikerRUS ! I just updated to latest |
/gha run r-solaris Workflow Solaris CRAN check has been triggered! 🚀 solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.3.1.99.tar.gz-c943c7d3a0a846faa86af99f52d0b1c2 |
/gha run r-valgrind Workflow R valgrind tests has been triggered! 🚀 Status: success ✔️. |
Awesome @jameslamb ! We can soon start adding more vignettes. Regarding naming: On CRAN, vignettes are sorted by the file name of the corresponding .rmd file, not by the vignette title or its An example from our
Thus, we can try to use names like "v1_basic_classification.rmd" etc. |
thanks for the great information @mayer79 ! I just responded over at #1944 (comment). Let's move the conversation there for greater visibility. |
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. |
Description from previous PR
Description copied from #3946. See #3946 (comment) for an explanation of why #3946's changes were moved to this PR.
First step towards #1944.
This initial vignette shows the very basic workflow of LightGBM. As such, it would not replace the demo with the corresponding name as that seems rather a guide how to deal with sparse data.
The html looks as in this zip here:
basic_walkthrough.zip
Some notes on R vignettes in general
Notes for Reviewers
See #3946 (review) for some relevant information needed to review this PR.