chore(ingestion): migrate to ruff for format + isort + unused-import#27739
chore(ingestion): migrate to ruff for format + isort + unused-import#27739
Conversation
- Swap formatter + import-sorter + unused-import tooling for ruff (line-length 120, target py3.10) in ingestion + openmetadata-airflow-apis - Drop dead [tool.mypy] config; basedpyright is the active type checker - Bump requires-python to >=3.10 to match noxfile and CLAUDE.md (3.9 is documented as broken on Mac in noxfile.py) - Bump pre-commit-hooks v2.3 -> v5.0; the new check-json catches four pre-existing JSON issues now excluded with an inline TODO - Update Makefile py_format / py_format_check targets to call ruff
- 253 noqa markers added via 'ruff check --add-noqa' across 128 files, freezing existing violations so this PR is a tooling-only swap. Per-rule cleanup tracked in the TODO comment in ingestion/pyproject.toml. - Bulk reformat from black 22.3 -> ruff format @ line-length 120. Cosmetic only: imports balanced (-32/+32), structural keywords balanced (-2221/+2221), no logic changes. - Star-import rules (F403/F405) globally ignored; refactoring wildcard imports across connectors is a separate effort.
github-actions
Bot
added
Ingestion
safe to test
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| service_type = get_service_type(source_type) | ||
| connection_class = get_connection_class(source_type, service_type) | ||
|
|
||
| if source_type in HAS_INNER_CONNECTION: |
| logger.error( | ||
| f"Secret value [{secret_id}] not present in the configured secrets manager: {exc}" | ||
| ) | ||
| logger.error(f"Secret value [{secret_id}] not present in the configured secrets manager: {exc}") |
| logger.info( | ||
| f"this is user: {user}, password: {password}, text: {text_response}" | ||
| ) | ||
| logger.info(f"this is user: {user}, password: {password}, text: {text_response}") |
| logger.error( | ||
| f"Could not get the secret value of {secret_id} due to [{exc}]" | ||
| ) | ||
| logger.error(f"Could not get the secret value of {secret_id} due to [{exc}]") |
| raise exc | ||
|
|
||
| def load_credentials(self) -> Optional["AzureCredentials"]: | ||
| def load_credentials(self) -> Optional["AzureCredentials"]: # noqa: F821 |
| logger.error( | ||
| f"Could not get the secret value of {secret_id} due to [{exc}]" | ||
| ) | ||
| logger.error(f"Could not get the secret value of {secret_id} due to [{exc}]") |
| logger.error( | ||
| f"Could not get the secret value of {secret_id} due to [{exc}]" | ||
| ) | ||
| logger.error(f"Could not get the secret value of {secret_id} due to [{exc}]") |
| logger.error( | ||
| f"Could not get the secret value of {secret_id} due to [{exc}]" | ||
| ) | ||
| logger.error(f"Could not get the secret value of {secret_id} due to [{exc}]") |
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
- filters.py: drop redundant parens around re.match(...) in `if` (C0325 superfluous-parens) — exposed when ruff format unwrapped them - nosql_adaptor.py: move `# pylint: disable=unused-argument` from the `column:` line to the `def` line so it covers `table` too (W0613) — scope was line-based, lost when ruff split params onto multiple lines - action1xx.py: replace `arguments-differ` with `signature-differs` in the disable directive (was always wrong code) and drop the now-useless `unused-argument` suppression (I0021)
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
Conflicts resolved by taking main's content (8 files) and re-applying ruff format + ruff check --add-noqa to keep the modernization PR behavior-preserving.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| def patch_column_tags( | ||
| self, | ||
| table: ClassifiableEntityType, | ||
| column_tags: List[ColumnTag], | ||
| operation: Union[ | ||
| PatchOperation.ADD, PatchOperation.REMOVE | ||
| ] = PatchOperation.ADD, | ||
| operation: Union[PatchOperation.ADD, PatchOperation.REMOVE] = PatchOperation.ADD, | ||
| ) -> Optional[T]: |
There was a problem hiding this comment.
💡 Quality: patch_column_tags parameter still named table for Container entities
After generalizing patch_column_tags to accept ClassifiableEntityType (Table or Container), the parameter is still named table (line 495) and documented as such. Downstream callers like metadata_rest.py:823 pass table=entity where entity is a Container. This is confusing and will make the code harder to maintain. Consider renaming to entity to match the actual semantics.
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
…ions CI's `make py_format_check` runs from the repo root and passes both `ingestion/` and `./openmetadata-airflow-apis/` to ruff in a single invocation. With multiple root paths, ruff's parallel file discovery races on extend-exclude matching against the project root, so files under `ingestion/src/metadata/generated/` were intermittently scanned and produced ~830 I001 violations. 20-run repro: 10/20 fail without the fix, 20/20 pass with the fix. Each excluded directory now appears twice in extend-exclude: - the project-root-relative pattern (cwd = ingestion/) - the prefixed pattern (cwd = repo root, multi-root invocation)
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
…isable - openmetadata-airflow-apis/pyproject.toml: switch coverage to module-name source + [tool.coverage.paths] glob remap (matches the ingestion pattern). Drops the hardcoded `env/lib/python3.9/site-packages/...` source path, which broke after the requires-python bump to 3.10. (Finding 1) - ingestion/setup.py: remove dead python_version<'3.9' / >='3.9' guards on mysql-connector-python and testcontainers; promote locust to a regular test dep (was conditionally added under sys.version_info >= (3, 9)). Also remove the now-unused `import sys`. (Finding 3) - ingestion/src/metadata/great_expectations/action1xx.py: cover both arguments-differ (great_expectations 0.18.x parent) and signature-differs (great_expectations 1.x parent) in the pylint disable comment, since CI installs 0.18.x and local often has 1.x. unused-argument covers the unused action_context. The opposite rule fires as I0021 useless-suppression on each environment, which is informational and does not affect pylint's exit code.
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
🟡 Playwright Results — all passed (18 flaky)✅ 3955 passed · ❌ 0 failed · 🟡 18 flaky · ⏭️ 86 skipped
🟡 18 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
Auto-fixes from `pre-commit run --all-files` after merging main into slice 1 (the main commits include the ruff migration #27739/#27774). Changes: - Drop string-quoted forward references where `from __future__ import annotations` is in effect (auto-fix). - Move type-only imports under `TYPE_CHECKING` blocks (auto-fix). - Convert `Union[A, B, ...]` to `A | B | ...` in `pipelines.py`. - Annotate class-level mutable defaults with `ClassVar` in `entity_assert.py` and `table_assert.py`. - Add `check=False` to `subprocess.run` in `cli_runner.py`. - Mark `StructuralMismatch` and `SourceBaselineDrift` with `# noqa: N818` — public exception names, intentional API surface. `pyproject.toml` per-file-ignores extended for `tests/cli_e2e_v2/**`: - `TID252` (relative imports) — by design (connector-centric layout). - `T201` (print) — top-level conftest's session posture banner. Both paths listed twice (relative + ingestion-prefixed) per the existing dual-cwd pattern in this file.
Summary
Migrates the Python tooling for `ingestion/` and `openmetadata-airflow-apis/` from black + isort + pycln to ruff (one tool, one config block, ~100x faster). Strictly a tooling swap — no logic changes.
What changed
Tooling config (commit 1: 5 files)
JSON files excluded from check-json
The pre-commit-hooks v5.0 `check-json` is stricter than v2.3 and catches four pre-existing JSON issues. Excluded with an inline TODO for separate cleanup:
Code changes (commit 2: 1568 files)
` markers added via `ruff check --add-noqa` across 128 files. Existing violations grandfathered so the migration is behavior-preserving. Per-rule cleanup tracked in the TODO comment at the top of `[tool.ruff.lint]` in `ingestion/pyproject.toml`:- F541 (52) — `f"hello"` → `"hello"`
- F402 (2) — loop-var-shadows-import
- F401 (13) — unused imports
- I001 (27) — import sorting
- F811 (16) — redefined-while-unused
- F841 (91) — unused-variable
- F821 (50) — undefined-name (likely real bugs to investigate)
What is not in scope (deliberate, follow-up PRs)
Sanity check
The reformat is large (1573 files, -24K net lines from line-length 120 unwrapping multi-line constructs) but structurally balanced:
Every import and structural keyword removed has a corresponding addition. No logic loss.
Summary by Gitar
python_versionpins insetup.pyformysql-connector-python,testcontainers, andlocust.[tool.coverage.paths]toopenmetadata-airflow-apis/pyproject.tomlto ensure consistent path resolution in CI.pylintsuppression forarguments-differ,signature-differs, andunused-argumentinOpenMetadataValidationAction1xx.This will update automatically on new commits.