Fixes #30696: feat(ingestion): enforce import layering and import-cost ceilings - #30697
Fixes #30696: feat(ingestion): enforce import layering and import-cost ceilings#30697IceS2 wants to merge 6 commits into
Conversation
…t ceilings Phase 0 (#30376) cut `import metadata` peak RSS with defer_build, but nothing stops it growing back: it went from ~261 MB (1.9) to ~435 MB (1.13) unnoticed and OOM-killed 512Mi automation pods. Add the guardrails. scripts/check_import_layers.py enforces a layering (leaf < client < framework < connectors < entrypoints): a module must not import a higher layer at module scope, because that drags the higher layer's subtree into every process. The 94 existing violations are recorded in import_layers_baseline.json so they can be burned down without regressing; the check fails on new violations and on stale baseline entries. tests/unit/test_import_cost.py caps `import metadata` peak RSS and module count, and tests/unit/test_import_layers.py gates the contract in CI. Analysing the AST rather than import-linter is deliberate: most metadata.* subpackages are PEP 420 namespace packages (Collate's ingestion-extension merges connectors into them from a separate distribution), which grimp cannot see, and only module-scope imports cost import time whereas grimp also counts function-local ones. Stop treating lazy imports as a smell: PLC0415 is now ignored, since function-local imports are the tool for keeping heavy subtrees out of import time, and the 267 noqa directives it required are removed.
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
github-actions
Bot
added
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
|
Too many files changed for review. ( Bypass the limit by tagging |
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
✅ Playwright Results — workflow succeededValidated commit ✅ 597 passed · ❌ 0 failed · 🟡 2 flaky · ⏭️ 3 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 50m 46s ⏱️ Max setup 2m 55s · max shard execution 14m 28s · max shard-job elapsed before upload 21m 36s · reporting 6s 🌐 209.12 requests/attempt · 2.77 app boots/UI scenario · 5.99% common-shard skew Optimization targets still in progress:
🟡 2 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 |
Replaces the hand-rolled AST checker with import-linter, which does the job properly: it analyses the same 2404 modules, reports the indirect chain behind a violation rather than just the direct edge, and is configuration rather than code we maintain. The earlier conclusion that import-linter could not see this tree was wrong. It can, as long as root_packages names every PEP 420 namespace portion under metadata explicitly — metadata itself is a regular package and grimp does not descend into namespace directories nested inside one. Naming only `metadata` finds 166 modules; naming the portions finds 2404. Siblings within a layer are joined with ':' so they may import each other. The 60 existing violations are recorded in ignore_imports as the burn-down baseline; tests/unit/test_import_layers.py gates the contract and also asserts root_packages stays exhaustive, since an unlisted namespace portion would be silently unanalysed. Note the tradeoff: import-linter counts function-local imports as edges, so the baseline includes imports that are already deliberately lazy (for example custom_pydantic -> secrets_manager_factory, which is function-local to break a cycle). Import *cost* is measured separately by tests/unit/test_import_cost.py.
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
Review of the contract turned up four things. The ignore list was not minimal: five entries were only chain starts, and are covered by ignoring the edge that actually crosses a layer. Each remaining entry was checked by removing it and confirming the contract then fails, so the list is now 56 entries with nothing redundant. metadata.ingestion.progress and metadata.ingestion.diagnostics sat outside every layer, which left imports to and from them unconstrained. Layering them surfaced a real chain (progress -> models.topology -> ingestion.source) that was previously invisible. Only metadata.ingestion.models stays unlayered, because custom_pydantic belongs below metadata.generated while the rest of the package is framework-level; test_every_ingestion_subpackage_has_a_layer now fails if any other subpackage drifts out of a layer. The ignore list mixed two different things: imports that are already function-local, which import-linter cannot recognise and which will never be removed, and module-scope imports that should be. They are now separate sections, so the second one reads as the list of work it is. Comments and docstrings carried release-by-release RSS figures and incident detail that belongs in the issue, not in the tree; they are cut back to the contract and the reason a reader would need.
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
Declare metadata as a container so layers read as `ingestion.source` rather than `metadata.ingestion.source`; each layer now fits on one line. Group ignore_imports by why the import exists rather than listing 56 lines flat: imports that are already lazy, shared helpers that grew upwards, the ometa client carrying framework types, feature packages bound to individual connectors, and ingestion.models spanning layers. Five headings instead of a comment per entry. The entry set is unchanged. Restore the instruction that the list is fixed by removing entries, not extended, which was lost when the sections were introduced.
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
The ignore list had a section for imports that are already function-local, described as permanent because they cost nothing at import time. That was the wrong distinction: deferring an import hides its cost, not the dependency, so a function-local import across a layer is still the same design problem. Those seven entries move into the groups they belong to by cause. All 56 now read as one list of debt to remove.
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
…change py_format_check lints ingestion/ and openmetadata-airflow-apis/ together, so ignoring PLC0415 left 80 unused noqa directives in the second tree and RUF100 failed the build.
Code Review ✅ Approved 2 resolved / 2 findingsEnforces import layering and import-cost ceilings using import-linter and subprocess memory caps, addressing the layer checker under-detects and coverage test findings. ✅ 2 resolved✅ Edge Case: Layer checker under-detects some module-scope import forms
✅ Edge Case: Coverage test only guards top-level metadata subpackages
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source |
Fixes #30696.
#30376 cut
import metadatapeak RSS by deferring the generated-model pydantic build. This adds the guardrails so the cost cannot grow back silently, and so the remaining lazy-import work has something to measure against.What's added
Layering contract —
ingestion/.importlinterdeclares a layering (generated< client < framework <ingestion.source< entrypoints) andimport-linterfails when a module imports a higher layer, because that drags the higher layer's subtree into every process that imports the lower one.The 56 existing violations are recorded in
ignore_importsas the burn-down baseline, so today's tree passes. They are grouped by cause rather than annotated one by one: shared helpers (utils,mixins,config) that grew upwards, framework and connector types carried on the ometa client's mixins, feature packages bound to individual connectors, andingestion.modelsspanning layers. Each entry was checked by removing it and confirming the contract then fails, so the list has nothing redundant in it.Import-cost ceilings —
ingestion/tests/unit/test_import_cost.pycapsimport metadatapeak RSS (480 MB) and module count (5000), measured in a subprocess. Deliberately loose: they catch a step change, not run-to-run noise. Verified they bite — withOM_PYDANTIC_DEFER_BUILD=0(simulating adefer_buildrevert) the RSS test fails at 509 MB.tests/unit/test_import_layers.pygates the contract in CI and additionally assertsroot_packagesstays exhaustive (see below).PLC0415is now ignored (import-outside-top-level). It made every deliberate lazy import carry anoqa, which is backwards here — function-local imports are the tool for keeping heavy subtrees out of import time. The 267 directives it required are removed (that is the bulk of the diff: comment-only, 117 files).Configuring import-linter for this package
root_packageshas to name every PEP 420 namespace portion undermetadataexplicitly.metadataitself is a regular package, and grimp does not descend into namespace directories nested inside one — so naming onlymetadataanalyses 166 modules, while naming the portions analyses 2404. Those subpackages are namespace packages deliberately: the Collate ingestion-extension merges connectors intometadata.ingestion.source.*from a separate distribution, so adding__init__.pyis not an option.Because that is easy to get wrong,
test_every_metadata_subpackage_is_analysedfails if a new top-level subpackage is neither declared nor explicitly excluded.Layer siblings are joined with
:(not|) so modules within a layer may import each other.Function-local imports still count. import-linter treats a deferred import as a dependency, which is what we want: making an import lazy hides its cost, not the dependency direction, so it cannot be used to get around a layer. Import cost is a separate concern, measured by the ceilings above.
Verification
make check_import_layerspasses (Contracts: 1 kept, 0 broken, 2404 files / 10861 dependencies analysed). Adding ametadata.timer -> metadata.ingestion.sourceimport fails it with the rule and the offending edge (exit 1); removing it passes (exit 0).ruff checkandruff format --checkclean overingestion/.tests/unit/{models,connections,core,utils}slice: 1 failed, 457 passed — identical counts on a clean tree, so the one failure (test_status_warning_handler.py::TestStepHandlerAttachment::test_warning_inside_run_scope_populates_status, order-dependent) is pre-existing and unrelated.Follow-ups this unblocks
Making the SQL-lineage parser import lazy, string-dispatching the secrets-manager backends, and moving driver/profiler imports into functions — each removes baseline entries and shows up against the ceilings.