-
Notifications
You must be signed in to change notification settings - Fork 65
post-processing: prevent early exit in customization #664
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
Conversation
✅ Deploy Preview for docs-rapids-ai ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@jameslamb What do you think about a README.md in the I believe this information would be helpful moving forward for everyone who will do similar work here. |
|
|
||
| echo "Customizing $(wc -l < ${MANIFEST_FILE} | tr -d ' ') HTML files" | ||
| python ${SCRIPT_SRC_FOLDER}/customize_doc.py "${MANIFEST_FILE}" | ||
| python -u ${SCRIPT_SRC_FOLDER}/customize_doc.py "${MANIFEST_FILE}" |
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.
For those finding this PR from search... adding -u ensures that Python stdout / stderr are printed immediately ("unbuffered").
I found that the warning about unrecognized sphinx themes, which goes to stderr, was not being printed immediately after the log messages about that same file printed to stdout.
Removing buffering fixes that.
|
Thanks for the quick review!
There is already https://github.com/rapidsai/docs/blob/main/ci/customization/README.md for the "customization" part, but it depends on other things not in that Sure! I'll update / add docs in a follow-up PR. I'll merge this so we can get feedback from another deployment. |
|
Ok yep this worked! Build link: https://github.com/rapidsai/docs/actions/runs/16885825806/job/47833203056 I see several places where this warning was emitted but didn't stop the build, for example: and And the drop-down selectors and other things look correct (checked after clearing my browser cache, to be sure):
The post-processing took about 20 minutes and the entire deployment took about 30 minutes... so this still cut the deployment time by 30-40 minutes relative to what it was before this set of PRs 😁 |
Great work! |
After merging #663, the post-processing "customization" process that updates things like drop-down version selectors is not processing all of the files it should be.
See #663 (comment) for details.
This fixes that.
Notes for Reviewers
Speed?
In #663, I said that I saw a post-processing process that's been taking around 1 hour in CI take around 4 minutes for me locally. That was an over-estimate influenced by this accidentally stopping early 😭
Following the testing steps added in #659, with the fixes in this PR it looks like this process will now take more like 20 minutes.
So still noticably faster, even if not as fast as I'd though before 😅
How was this not caught in testing?
In the logs I see in the build linked from #663 (comment),
raftis the first library processed and it immediately hits this issue.When I run this locally, several other libraries are processed before that, and don't hit this issue.
Locally this prints thousands of lines of logs and exited with 0, so I just assumed it was working correctly and didn't carefully check those logs.
I suspect the difference in order is a result of different Python versions... no guarantees are made about the ordering of dictionary keys, and this code is driven by some YAML/JSON config files converted to Python dictionaries.