openpilot icon indicating copy to clipboard operation
openpilot copied to clipboard

Speeding up docs build in CI using caching.

Open dpeachpeach opened this issue 1 year ago • 10 comments

FOR ISSUE #32687

Description

This speeds up the 'Docs Build' step of the docs build job by caching the results of building the docs from prior actions.

This kind of 'skips' the step 'build docs' altogether (in the successful case where there is a cache hit) and doesn't really provide any improvement to speeding up building the docs in case they were modified.

I didn't know if that's what you guys were going for but I thought it could be as a decent amount of the contributions made don't affect building the docs directly. In those cases it speeds up the action by eliminating building the docs entirely. (Drawback here is another element present in the cache fields.)

I was also thinking of other approaches to speed things up. If this isn't sufficient / what you're looking for I can find a different approach and improve it further! Give me any feedback you have I'll go back to work on this.

Verification

I tested the action on my own fork, which showed that it cached correctly and provided an elimination of the 'build docs' step in case of cache hit. It might take two runs to store the cache properly but barring that it seems to be a success.

dpeachpeach avatar Jun 17 '24 01:06 dpeachpeach

Thanks for contributing to openpilot! In order for us to review your PR as quickly as possible, check the following:

  • Convert your PR to a draft unless it's ready to review
  • Read the contributing docs
  • Before marking as "ready for review", ensure:
    • the goal is clearly stated in the description
    • all the tests are passing
    • the change is something we merge
    • include a route or your device' dongle ID if relevant

github-actions[bot] avatar Jun 17 '24 01:06 github-actions[bot]

This is close but not quite right.

We want to build able to incrementally build using the cache, like we can with our scons build cache.

adeebshihadeh avatar Jun 18 '24 05:06 adeebshihadeh

I took your feedback and tried to improve it further. I think I've got it pending your review but I just wanted to provide some explanation behind the edits + broad strokes bullet points:

  • It turns out Sphinx already implements caching implicitly! The reason why it didn't manifest itself in building the docs for comma was that the build directory would be deleted on every build. That is where the automatically generated caches (or pickles) (.doctrees) were stored. I stopped that behavior and now it runs much faster.
  • I also added in parallelism at the sphinx-build call, but when I had this undergo testing on my fork it didn't change anything about the performance (probably because parallelism is already done through the github action). This can be removed to clean up the code.
  • I went through the route of looking at some optimizations that could affect the theme but I figured those were design decisions and I didn't want to intrude.
  • I changed the paths of some of the directory alias definitions to more closely follow definitions I saw in the docs.

When undergoing testing on my fork, this seems to have resulted in the same results with faster compilation time. Let me know if you have any more feedback!

dpeachpeach avatar Jun 19 '24 23:06 dpeachpeach

Sounds good, I'll review once you mark it as ready for review.

  • I went through the route of looking at some optimizations that could affect the theme but I figured those were design decisions and I didn't want to intrude.

We basically got it setup and haven't touched it since, so design changes are welcome!

adeebshihadeh avatar Jun 19 '24 23:06 adeebshihadeh

I see a whole bunch of warnings and errors; are those not new?

https://github.com/commaai/openpilot/actions/runs/9589108324/job/26442355846?pr=32766

adeebshihadeh avatar Jun 19 '24 23:06 adeebshihadeh

I cross-referenced those a bit previously and they seem to be the same warnings / errors that were present in the sample you showed in the issue: https://github.com/commaai/openpilot/actions/runs/9454981147/job/26043750673

However I was worried about that. I was going to try to debug a little bit to see if any of those were related to my implementation before marking it ready for review.

dpeachpeach avatar Jun 19 '24 23:06 dpeachpeach

Just found a serious issue @adeebshihadeh, going to have to work on this a bit more. Sorry for prematurely setting up the PR!

dpeachpeach avatar Jun 19 '24 23:06 dpeachpeach

No worries, take your time :)

You can also share your new docs on Discord and ask people to poke around for bugs

adeebshihadeh avatar Jun 19 '24 23:06 adeebshihadeh

All right, fixed the issue! When I test the generated html through local hosting, everything seems equivalent. (Compared it side by side with the docs available online). I'll make a post on the discord for anyone to take a look as well but otherwise I think this is ready!

dpeachpeach avatar Jun 20 '24 00:06 dpeachpeach

This PR has had no activity for 14 days. It will be automatically closed in 3 days if there is no activity.

github-actions[bot] avatar Jul 06 '24 01:07 github-actions[bot]

This PR has been automatically closed due to inactivity. Feel free to re-open once activity resumes.

github-actions[bot] avatar Jul 09 '24 01:07 github-actions[bot]