-
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
[ci] Fix version matching between RTD pages and R-package pages #6673
Conversation
@jameslamb Could you please enable RTD builds for this branch for testing? |
Sure! Done. The first build just started: https://readthedocs.org/projects/lightgbm/builds/25905740/ |
Thank you! |
One more issue is that |
I believe it's ready for reviews. |
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.
Thanks for this! I don't know much about Javascript but I clicked around https://lightgbm.readthedocs.io/en/ci-rtd-r-version/R/reference/ and https://lightgbm.readthedocs.io/en/ci-rtd-r-version/ and the changes look good to me.
On vague idea to consider... could any of this version-replacement be done at build time instead of runtime, with sed
or similar?
I did that recently in another project's sphinx Makefile
and found it worked well:
If that's possible, then it'd be nice for a few reasons:
- less Javascript to maintain (which we don't have a lot of maintainer experience with)
- docs site would load faster and more reliably
I'm marking this "Approve" because I support merging this and don't want these question to block it.
@jameslamb Hey, great idea about build time changes! I'll try to implement it in This PR isn't critical, so I think we shouldn't merge the current fix. |
Looks like RTD really treats @jameslamb |
hmmmm I don't understand that. Because at https://readthedocs.org/projects/lightgbm/versions/, it looks to me like readthedocs recognizes that But I do see that the "Edit on GitHub" button at https://lightgbm.readthedocs.io/en/stable/ links to https://github.com/Microsoft/LightGBM/blob/d73c6b530b39a18a3cacaafc4e42550be853c036/docs/index.rst (which is v4.0.0). That suggests that this issue isn't limited to just the R package. I just manually triggered a build of build link: https://readthedocs.org/projects/lightgbm/builds/25960601/ Let's see if that fixes it. If it does, then maybe we have to manually trigger a build for |
This PR is ready for new review. |
Seems that helped! Maybe the following code is related? Lines 142 to 148 in 668bf5d
Ref: readthedocs/readthedocs.org#10935 (comment) readthedocs/readthedocs.org#11183 and other issues in RTD repo with stable in the title: https://github.com/search?q=repo%3Areadthedocs%2Freadthedocs.org+stable&type=issues
|
This all looks great to me @StrikerRUS , thank you for your hard work! I think the links you shared are very relevant, especially that code link showing the docs build reading It seems to me that this PR plus manually rebuilding stable on every release should be enough. Please merge this whenever you want. |
I also just added an about triggering a new |
@jameslamb Thanks! Yeah, I believe that manual triggering is only the option for us for now, unfortunately. |
@jameslamb And I'd like to ask you to remove |
done! |
Maybe you need to refresh your browser cache? I don't see it when I navigate to https://lightgbm.readthedocs.io/en/latest/index.html Or in the project settings at https://readthedocs.org/projects/lightgbm/versions/ |
Thanks for the suggestion! |
I believe this element selectors are quite unreliable and subject to frequent changes.OK, it's shadow DOM, so we simply cannot access it.
Unfortunately, this fix will be active only with next release and is not backported to previous versions.