chore: Modernize Python tooling (ruff, uv, mypy)#8040
Open
phacops wants to merge 9 commits into
Open
Conversation
Bring the lint/type toolchain up to modern standards and clear the debt the upgrades surface. Ruff: expand the lint set from E/F/W/I to add B (bugbear), UP (pyupgrade), C4 (comprehensions), SIM (simplify) and RET (return), enforce E402, and set isort known-first-party. Apply the resulting ~5,100 fixes (typing modernization, comprehension/return cleanups). PEP 695 native-generic rules (UP040/046/047) are deliberately left ignored - mass-migrating generics under custom metaclasses is a separate, risky change. mypy: upgrade 1.1.1 -> 2.1.0 (mypyc-compiled). The jump surfaced 110 pre-existing errors that 1.1.1 missed; all are fixed (41 redundant casts plus arg-type/attr-defined/type-arg across ~25 files). Resolve the duplicate-conftest collision and align the pre-commit mypy hook's excludes with the config so 'mypy .' and pre-commit check the same files. type: ignore: reduce from 159 to 62 by fixing the underlying types; every remaining suppression is now error-code-specific (zero bare, zero unused). Config: move pytest config from setup.cfg (deleted) into pyproject.toml, add Makefile lint/lint-check/typecheck targets, and remove the obsolete no-op tests/utils/conftest.py. Several latent bugs were caught in the process: missing test assertions, a profile-events call missing an argument, and DeletionSettings constructed with positional args in the wrong fields. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Agent transcript: https://claudescope.sentry.dev/share/DD3ny1HpZNB23VAt3zOpAjqDbo3ncHFJvSHXcXKnr-Y
…hon-tooling # Conflicts: # snuba/subscriptions/executor_consumer.py # snuba/web/rpc/common/common.py # snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py # tests/datasets/validation/test_datetime_condition_validator.py # tests/subscriptions/test_executor_consumer.py Agent transcript: https://claudescope.sentry.dev/share/RDuC02F7kx3tyuNYFjVm7HpwWU0lsYKJKTLNeWGOOOM
MeredithAnya
approved these changes
Jun 25, 2026
Resolve conflicts and reconcile the modernization sweep with new code that landed on master: - web/delete_query.py: keep master's count_storage_key / reader_storage_key row-count logic; modernize its new signature to `StorageKey | None`. - web/rpc/common/common.py: keep master's `membership_as_has` threading through trace_item_filters_to_expression while preserving the RET lint fix (elif-after-return -> if). - environment.py / tests/conftest.py: master's merged code used legacy typing (`Optional`, `List`) whose imports the sweep had removed; switch to `X | None` / `list[...]`. - tests/web/test_bulk_delete_query.py: collapse master's nested `with` into a single parenthesized context (SIM117). Also fixes the test/admin/test_system_queries.py CI failure: the sweep tightened `pytest.raises(Exception)` to `pytest.raises(InvalidCustomQuery)`, but these malformed/non-SELECT system queries are rejected by ClickHouse at EXPLAIN time (ClickhouseError), not by Snuba's own validation. Accept either legitimate rejection type instead of bare Exception. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Fi8s8SofmyLTG3fWgq9ad
The sweep fixed the production `gather_profile_events` call to pass `clusterless_mode` (the arg it was previously omitting, shifting `g.user` into the wrong position), but the mock assertion in test_gather_profile_events still expected the old 6-arg call. Update it to expect the extra `False` (clusterless_mode) so the assertion matches the fixed call. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Fi8s8SofmyLTG3fWgq9ad
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
ddl-changes: the sweep reformatted historical migration files, so the DDL-diff job re-renders them. A few migrations (e.g. functions 0001) build their SQL from a live ClickHouse version lookup, which the job has no node for. Catch ClickhouseError per migration and emit a `-- skipped` note instead of failing the whole diff. Also a general robustness fix: editing any old version-dependent migration would otherwise break this check. test_cluster: test_cache_partition and test_query_settings_prefix were bare comparisons with no `assert` on master (so they never actually checked anything). The sweep correctly added `assert`, which exposed that neither test reloads the cluster module after patching settings.CLUSTERS with FULL_CONFIG, so get_cluster() still saw the original config and cache_partition_id/query_settings_prefix came back as None. Add the importlib.reload(cluster) call the other FULL_CONFIG tests already use. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Fi8s8SofmyLTG3fWgq9ad
…test ddl-changes: with migrations kept in the sweep, the resilience fix let the script run but it then rendered all ~267 reformatted migrations, and the DDL bot comment exceeded GitHub's 65536-char limit. Switch the diff filter from AM to A so only newly *added* migrations are rendered (the "new DDL changes" the check is named for). Shipped migrations are immutable, so a formatting-only touch should not re-render them. The per-migration ClickhouseError guard stays as a safety net for added version-dependent migrations. test_storage_query_identity_translate: the sweep replaced a single type-ignored assert with three nested isinstance asserts to drop the ignore, but the middle type was wrong. try_translate_storage_query wraps the inner query via identity_translate, so storage_query.get_from_clause() is a ClickhouseQuery (CompositeQuery -> ClickhouseQuery -> Table), not a CompositeQuery. Assert ClickhouseQuery. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Fi8s8SofmyLTG3fWgq9ad
TEMPORARY — will be reverted to `-x`. Lets one CI run report the full list of remaining latent test failures (instead of one per run) so they can be fixed in a single batch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Fi8s8SofmyLTG3fWgq9ad
Only conflict was snuba/web/views.py: master (#8109, memory reductions in the result/response path) and this branch both landed _sanitize_payload_in_place. The functions are identical except master uses elif/else where the sweep's RET rules use if + bare return. Keep the RET-compliant form (functionally identical). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Fi8s8SofmyLTG3fWgq9ad
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Modernizes the Python lint/type toolchain and clears the debt the upgrades surface. Functionally a no-op for production code, but it expands static-analysis coverage and upgrades mypy across a major version. Reviewable as: tooling/config diffs (small) + a large mechanical sweep (autofixes) + targeted type fixes.
Ruff: expanded rule set
Adds
B,UP,C4,SIM,RETto the existingE/F/W/I, enforcesE402(was blanket-ignored, which hid misplaced imports), and sets isortknown-first-party. The ~5,100 resulting fixes are mostly mechanical typing modernization (Optional[X]→X | None,List→list). PEP 695 native-generic rules (UP040/046/047) are deliberately left ignored — mass-migrating generics under custom metaclasses is a separate, riskier change.mypy: 1.1.1 → 2.1.0
A three-year version jump (mypyc-compiled). It surfaced 110 pre-existing errors that 1.1.1 silently missed — all fixed (41 redundant casts plus
arg-type/attr-defined/type-argacross ~25 files). Also resolves the duplicate-conftestcollision somypy .runs clean, and aligns the pre-commit mypy hook's excludes with the config (pre-commit passes filenames explicitly, which bypassed mypy's ownexclude).type: ignore: 159 → 62
The reduction comes from fixing the underlying types, not suppressing. Every remaining ignore is now error-code-specific (
# type: ignore[arg-type]) — zero bare, zero unused.Config & housekeeping
pytest config moves from
setup.cfg(deleted) intopyproject.toml;Makefilegainslint/lint-check/typechecktargets; the obsolete no-optests/utils/conftest.py(overriding a fixture that no longer exists) is removed.The stricter analysis also caught a few latent bugs, now fixed: test assertions that never asserted (bare comparisons), a
profile_eventscall missing itsclusterless_modeargument, andDeletionSettingsconstructed with positional args landing in the wrong fields.Agent transcript: https://claudescope.sentry.dev/share/1JX1N9F7KUr3uBfmc57-ubJQKrY3XFoIPufiNWeh91w
Linear: EAP-589