feat: Improve local dev experience with file-aware hooks and auto parallelization#5956
Conversation
There was a problem hiding this comment.
💡 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.
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.
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 👍 / 👎.
| - 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.
🟡 && 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.
…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>
- 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>
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>
3e5d42d to
6cf622e
Compare
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Apply the same fix as test-python-integration-local to preserve xdist_group markers for test isolation (Ray OOM prevention, database registry conflicts, etc). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
These Makefile targets were added but never wired up to pre-commit. Pre-commit passes filenames as positional arguments, but Make expects FILES="..." variable syntax. The inline bash commands in pre-commit work correctly, so these targets are dead code. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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