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

Improve the performance of the formatter instability check job #14471

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Nov 20, 2024

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:

  1. Making the formatter script idempotent, so it can be run repeatedly and is robust to changing commits
  2. Reducing the overhead of the git operations, minimizing the data transfer
  3. Parallelizing all the git operations by repository

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.

@zanieb zanieb added the ci Related to internal CI tooling label Nov 20, 2024
@zanieb zanieb changed the title Improve the performance of the formatter instability checks Improve the performance of the formatter instability check job Nov 20, 2024
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@zanieb zanieb marked this pull request as ready for review November 20, 2024 04:33
@MichaReiser
Copy link
Member

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?

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.

Copy link
Member

@MichaReiser MichaReiser left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Related to internal CI tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants