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

WIP: Shorten openpilot CI Setup Time to <40s #31836

Closed
wants to merge 15 commits into from

Conversation

BBBmau
Copy link
Contributor

@BBBmau BBBmau commented Mar 12, 2024

Fixes #30706

Based off of comments from the issue the best approach is removing dependencies after they're used with the help of poetry

Copy link
Contributor

github-actions bot commented Mar 12, 2024

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

@BBBmau
Copy link
Contributor Author

BBBmau commented Mar 12, 2024

25 seconds to restore apt-packages from cache. Gonna spend some time to see what exactly we don't need from here. run

@BBBmau
Copy link
Contributor Author

BBBmau commented Mar 13, 2024

I'm still working on this. After some research there really is no way to remove enough dependencies so that it's <20s

Caching is an issue due to needing to restore for every runner that's created when it's called. Having the ability to share runners across each job where setup-with-retry is mentioned would be really nice.

Which leads to what looks like is the best (and IMO only solution from what I've looked into) for getting it to <20s and that's setting up self-hosted runners with the openpilot image + dependencies. With comma using azure, I'll look into setting up runners that are hosted there. Regular docker containers work also where each container acts as a runner.

pinging since bounty hasn't been set to locked @adeebshihadeh

@gregjhogan
Copy link
Member

FYI, if the goal is to get the build time on PRs from forks under these time constraints, github says you should not use self-hosted runners in this scenario:
https://docs.github.com/en/actions/hosting-your-own-runners/managing-self-hosted-runners/about-self-hosted-runners#self-hosted-runner-security

@BBBmau
Copy link
Contributor Author

BBBmau commented Mar 13, 2024

FYI, if the goal is to get the build time on PRs from forks under these time constraints, github says you should not use self-hosted runners in this scenario: https://docs.github.com/en/actions/hosting-your-own-runners/managing-self-hosted-runners/about-self-hosted-runners#self-hosted-runner-security

My first thought to this is adding in permissions where self-runners are used on PRs from public forks only when approved or some kind of condition is met for contributors (contributors that have a long history in comma.ai repos or organization members)

Might be another hoop for maintainers to take in new contributors.

@gregjhogan thoughts? Just initial thoughts on how to get <20s possible.

https://docs.github.com/en/organizations/managing-organization-settings/disabling-or-limiting-github-actions-for-your-organization#configuring-required-approval-for-workflows-from-public-forks

@adeebshihadeh
Copy link
Contributor

We do not want to maintain self hosted runners and do not want any more hoops.

@BBBmau
Copy link
Contributor Author

BBBmau commented Mar 13, 2024

We do not want to maintain self hosted runners and do not want any more hoops.

back to the drawing board then.

@adeebshihadeh
Copy link
Contributor

Closing this since it's way too complicated. Feel free to open a new one if you come up with something simple

@BBBmau
Copy link
Contributor Author

BBBmau commented Mar 15, 2024

Closing this since it's way too complicated. Feel free to open a new one if you come up with something simple

Yep, working on something right now that's much simpler. I'll make a draft PR soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Speedup CI setup to <20s
3 participants