-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: Improve local dev experience with file-aware hooks and auto parallelization #5956
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
base: master
Are you sure you want to change the base?
Conversation
…allelization - Make precommit hooks file-aware: format and lint only changed Python files - Add conditional template hook that only runs when template files change - Update test-python-integration-local to use -n auto and --dist loadscope - Add new pytest markers: slow, cloud, local_only for better test selection - Add format-python-files and lint-python-files Makefile targets Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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.
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 423979c832
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
.pre-commit-config.yaml
Outdated
| name: Build Templates | ||
| stages: [commit] | ||
| language: system | ||
| files: ^infra/templates/|\.jinja2$ |
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.
Include roadmap inputs in template-hook file filter
Restricting the template hook to ^infra/templates/|\.jinja2$ means make build-templates will no longer run when docs/roadmap.md changes, even though infra/scripts/compile-templates.py reads that file to regenerate README.md. In that scenario, commits can update the roadmap without updating the generated README, leaving checked-in docs inconsistent until someone runs the build manually.
Useful? React with 👍 / 👎.
.pre-commit-config.yaml
Outdated
| - id: lint | ||
| name: Lint | ||
| types: [python] | ||
| entry: uv run ruff check --fix |
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.
Run Ruff formatter in the file-aware format hook
The new format-files hook only runs ruff check --fix, which does not apply ruff format styling. Since the paired lint hook also skips ruff format --check, commits can pass local pre-commit while still failing the repository's standard lint path (make lint-python) on formatting checks. This is a regression from the previous hook behavior that ran full formatting before commit.
Useful? React with 👍 / 👎.
- Format hook now runs both ruff check --fix AND ruff format - Lint hook now runs both ruff check AND ruff format --check - Template hook file filter now includes docs/roadmap.md Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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 codebase uses xdist_group markers extensively for test isolation (Ray OOM prevention, database registry tests, etc). The loadscope distribution mode ignores these markers, so restore loadgroup. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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.
| - id: lint | ||
| name: Lint | ||
| types: [python] | ||
| entry: bash -c 'uv run ruff check --fix "$@" && uv run ruff format "$@"' -- |
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.
🟡 && in format-files pre-commit hook skips ruff format when unfixable lint violations exist
The format-files pre-commit hook entry uses && to chain ruff check --fix and ruff format. When ruff check --fix encounters lint violations that cannot be auto-fixed, it returns exit code 1, and the && operator prevents ruff format from executing. This means files with unfixable lint issues will never be auto-formatted.
Root Cause and Impact
The entry at .pre-commit-config.yaml:13 is:
entry: bash -c 'uv run ruff check --fix "$@" && uv run ruff format "$@"' --
ruff check --fix returns exit code 1 when violations remain after applying all available auto-fixes (e.g., rules that have no auto-fix available). The && operator short-circuits, so ruff format is never invoked.
Notably, the corresponding format-python-files Makefile target at Makefile:64-65 correctly uses ; (semicolons) to separate the commands, ensuring ruff format always runs regardless of ruff check --fix's exit code:
uv run ruff check --fix $(FILES); \
uv run ruff format $(FILES); \Impact: When a developer commits Python files that have unfixable lint violations (which is common — many ruff rules are not auto-fixable), the formatting step is silently skipped. The developer's files won't be auto-formatted, defeating the purpose of the format hook.
| entry: bash -c 'uv run ruff check --fix "$@" && uv run ruff format "$@"' -- | |
| entry: bash -c 'uv run ruff check --fix "$@"; uv run ruff format "$@"' -- | |
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
test-python-integration-localto use-n autoand--dist loadscope(20-40% faster on machines with >8 cores)slow,cloud,local_only) for better test selection during developmentformat-python-filesandlint-python-filesMakefile targets for file-aware operationsTest plan
pytest --collect-only sdk/python/tests/unittime pre-commit run --all-filesto compare timingmake test-python-unit-fastto verify parallelization works🤖 Generated with Claude Code