-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Improve the performance of the formatter instability check job #14471
Conversation
bc249da
to
ccf977a
Compare
ccf977a
to
92e2aab
Compare
|
I'm still using them because they're not only useful to assess black compatibility (which the ecosystem checks don't give us) but it also assigns a number to how compatible the stable and preview style are. The latter is mainly useful to understand: Is this a change happening very locally in a single file or does it touch every file in a repository. Something that I find difficult to assess from the ecosystem checks (because they truncate). I'm also not concerned about the performance of this job. It only runs on formatter PRs and they're rare. Every formatter diff has to wait for the ecosystem check results, which tend to be much slower than any other check. |
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.
The initial motivation for idempotency was to include the repositories in the GitHub Actions cache. I'm not sure it's worth it yet — they're about 1GB and would consume our limited cache space. Regardless, it improves correctness for local invocations.
I don't think it's worth it. performance isn't a concern for this job and it runs to infrequently for the caches to even be useful.
Good to know you're still using it.
Not anymore! They run in 3m now — I'm following up on this because it's one of the slowest remaining jobs. |
I'm mainly saying that it's probably not worth your time. At least, I never waited for that job. |
We should probably get rid of this entirely and subsume it's functionality in the normal ecosystem checks? I don't think we're using the black comparison tests anymore, but maybe someone wants it?
There are a few major parts to this:
This reduces the setup time from 80s to 16s (locally).
The initial motivation for idempotency was to include the repositories in the GitHub Actions cache. I'm not sure it's worth it yet — they're about 1GB and would consume our limited cache space. Regardless, it improves correctness for local invocations.
The total runtime of the job is reduced from ~4m to ~3m.
I also made some cosmetic changes to the output paths and such.