-
Notifications
You must be signed in to change notification settings - Fork 741
Improve lintrunner job runtime #14240
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
|
Stack from ghstack (oldest at bottom): |
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/14240
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 Cancelled Jobs, 3 Pending, 5 Unrelated FailuresAs of commit d93113d with merge base e6b9111 ( CANCELLED JOBS - The following jobs were cancelled. Please retry:
FLAKY - The following jobs failed but were likely due to flakiness present on trunk:
BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
.github/workflows/lint.yml
Outdated
| RC=0 | ||
| # Run lintrunner on all files | ||
| if ! lintrunner --force-color --all-files --skip MYPY --tee-json=lint.json 2> /dev/null; then |
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.
I was thinking what if instead of --all-files can we run this on "changed" files in this PR as a required, not sure if that helps with MYPY performance or not..
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 problem isn't MYPY performance, it's the time to build and install executorch. check out some recent lintrunner runs on HUD: https://hud.pytorch.org/hud/pytorch/executorch/main/1?per_page=50&name_filter=lintrunner
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.
instead of --all-files can we run this on "changed" files in this PR
good idea, PyTorch's lint.yml does something like this, but it's out of scope for this PR
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.
on second thought, this is a good idea and I'm open to just increasing scope of this PR so that we can be confident that we actually get a speed improvement.
digantdesai
left a comment
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.
Stamping to unblock you from landing once ready
[ghstack-poisoned]
|
ugh, this only knocks 3 minutes off the time for the lintrunner job. I think we're better off just not running |
[ghstack-poisoned]
[ghstack-poisoned]
|
@digantdesai can you please rereview? this has had several major changes |
| PR_NUMBER="${{ github.event.number }}" | ||
| # Use gh CLI to get changed files in the PR with explicit repo | ||
| CHANGED_FILES=$(gh api repos/${{ github.repository }}/pulls/$PR_NUMBER/files --paginate --jq '.[] | select(.status != "removed") | .filename' | tr '\n' ' ' | sed 's/ $//') |
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.
Will —paginate give all the files at once or gives a page and ask you to go to next page?
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.
beats me. this is an exact copy from pytorch/pytorch
lintrunner is required to land, but it takes 10-15 minutes. This seems to improve on that by: - split out lintrunner-mypy job - copy pytorch/pytorch's changed files tracking - don't run `lintrunner init` unnecessarily
lintrunner is required to land, but it takes 10-15 minutes. This seems to improve on that by:
lintrunner initunnecessarily