-
Notifications
You must be signed in to change notification settings - Fork 33.4k
qa: fix ty caching and align CI with local run #46278
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
Changes from 4 commits
158bb9b
5693390
b6bb7f9
b406312
ec40f67
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,30 +15,14 @@ | |
| # - `check_args` below are the exact roots passed to `ty check`. | ||
| # - `cache_globs` here are only used by `utils/checkers.py` to decide when a | ||
| # previously clean `types` run can be reused from cache. | ||
| # Approximate: ty follows imports beyond the listed directories; these globs cover | ||
| # the explicitly targeted paths but not transitive dependencies. | ||
| # ty follows imports *beyond* the checked roots, so the cache key must cover every source file | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but the scope is always within
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, but we check a subset of the files in that directory with our set up. |
||
| # that could change a result -- not just the explicitly-checked paths. We hash the whole package | ||
| # (plus the standalone .circleci target) so any source edit busts the cache and forces a | ||
| # re-check. Otherwise a cached pass could silently hide a newly-introduced error in a | ||
| # transitively-imported module that ty pulls in but that isn't one of the checked roots. | ||
| "cache_globs": [ | ||
| "src/transformers/_typing.py", | ||
| "src/transformers/cli/**/*.py", | ||
| "src/transformers/modeling_utils.py", | ||
| "src/transformers/utils/**/*.py", | ||
| "src/transformers/generation/**/*.py", | ||
| "src/transformers/pipelines/__init__.py", | ||
| "src/transformers/pipelines/feature_extraction.py", | ||
| "src/transformers/pipelines/image_feature_extraction.py", | ||
| "src/transformers/pipelines/video_classification.py", | ||
| "src/transformers/quantizers/**/*.py", | ||
| "src/transformers/**/*.py", | ||
| ".circleci/create_circleci_config.py", | ||
| "src/transformers/dependency_versions_table.py", | ||
| "src/transformers/dependency_versions_check.py", | ||
| "src/transformers/conversion_mapping.py", | ||
| "src/transformers/time_series_utils.py", | ||
| "src/transformers/debug_utils.py", | ||
| "src/transformers/hyperparameter_search.py", | ||
| "src/transformers/pytorch_utils.py", | ||
| "src/transformers/file_utils.py", | ||
| "src/transformers/trainer_jit_checkpoint.py", | ||
| "src/transformers/trainer_optimizer.py", | ||
| ], | ||
| "check_args": [ | ||
| "src/transformers/_typing.py", | ||
|
|
@@ -74,8 +58,11 @@ def main(): | |
|
|
||
| directories = sys.argv[1:] | ||
| print(f"Running ty check on: {', '.join(directories)}") | ||
| # `--error-on-warning` makes ty exit non-zero on warning-level diagnostics (e.g. | ||
| # possibly-missing-attribute), not just errors. Without it, warnings print but ty exits 0, so | ||
| # `make typing` and CI both pass and the issue is never caught before commit. | ||
| result = subprocess.run( | ||
| ["ty", "check", "--respect-ignore-files", "--exclude", "**/*_pb*", *directories], | ||
| ["ty", "check", "--respect-ignore-files", "--error-on-warning", "--exclude", "**/*_pb*", *directories], | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Locally, we have this
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it was a combination:
the combo made the two environments behave differently when warnings where triggered. |
||
| ) | ||
| sys.exit(result.returncode) | ||
|
|
||
|
|
||
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 will leave @remi-or for this file
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.
it's done in the other PR that got merged. I have just rebased so its gone from this one