Add import contracts and a legacy-name check for packages - #1706
Merged
aacostadiaz merged 3 commits intoSep 1, 2026
Conversation
The frozen legacy package only works as a numerical oracle while the v1 stack cannot reach it. A legacy class that can be imported can be subclassed, re-exported and quietly ported, and then the oracle is no longer independent of the thing it judges. This turns that from a review aspiration into two gates. .importlinter holds five contracts: the v1 packages never import mace, mace never imports them, mace_core depends on no framework and no sibling, the torch and jax implementations never import each other, and mace_torch layers only import downwards. Each was checked by breaking it: adding an import of mace to mace_torch and of torch to mace_core breaks exactly two, naming the file and line, and both return to green when reverted. The layering contract marks every layer optional. mace_torch has none of them yet, and a contract naming a module that does not exist is simply broken, so the alternative was to add the gate after the imports it exists to prevent. The meta-lint catches the step before an import: a legacy symbol named in a v1 file at all. It matches the parsed syntax tree rather than the file's text, because the denylist contains MACE, which is also the project's name and appears in nearly every docstring in the tree. Identifiers, attributes, import targets and non-docstring strings all count, so getattr(module, "ScaleShiftMACE") is caught and a docstring mentioning MACE is not. The model-class list is checked against the classes legacy actually declares, so one added there fails here rather than becoming portable in silence. tests/architecture moves out of the unit job into the new one. It is the only job that installs both trees editable, which import-linter needs: it resolves each root package on the filesystem, so a missing one reads as a broken contract rather than a missing install. The runtime half of the guard already exists in mace_launcher.audit. Its second activation point is the tests/parity conftest, which PAR-1 creates.
aacostadiaz
temporarily deployed
to
gpu-internal
September 1, 2026 09:11 — with
GitHub Actions
Inactive
aacostadiaz
had a problem deploying
to
gpu-internal
September 1, 2026 09:11 — with
GitHub Actions
Error
aacostadiaz
had a problem deploying
to
gpu-internal
September 1, 2026 09:11 — with
GitHub Actions
Error
Closed
5 tasks
The name check collected references: identifiers, attribute access, import targets and non-docstring strings. It did not collect definitions, so a file declaring `class ScaleShiftMACE` and never mentioning the name again referred to nothing and passed. That is the wholesale copy the check exists to catch, and the easiest one to write. Class and function definitions now count, and the three spellings are covered by cases: a copied class, one subclassing another legacy class, and a function named after a legacy registry. Names v1 reuses are unaffected: `class BesselBasis` is still not flagged, because the exclusion is about which names are on the list, not about how they appear.
aacostadiaz
temporarily deployed
to
gpu-internal
September 1, 2026 09:22 — with
GitHub Actions
Inactive
aacostadiaz
temporarily deployed
to
gpu-internal
September 1, 2026 09:23 — with
GitHub Actions
Inactive
aacostadiaz
temporarily deployed
to
gpu-internal
September 1, 2026 09:23 — with
GitHub Actions
Inactive
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e564800. Configure here.
The test invoked `python -m importlinter.cli lint-imports`. That module has no `__main__` guard, so the subprocess imported it, did nothing and exited 0 with no output. The test asserted only on the return code, so it passed without checking a single contract. It now calls the console script, which exits 1 on a broken contract and 0 when all are kept, and it asserts the contract report is present before trusting the return code. The silent no-op returned 0 too, so a return code alone cannot tell a clean run from a run that never happened. The contracts also need every root package installed, since import-linter resolves each on the filesystem and reports a missing one as a broken contract rather than as a missing install. The test has no marker and is collected by any job running tests/, including the nightly jobs that install only the legacy tree, so it skips with the reason when a root is absent instead of failing for something that is not a violation.
aacostadiaz
temporarily deployed
to
gpu-internal
September 1, 2026 09:56 — with
GitHub Actions
Inactive
aacostadiaz
temporarily deployed
to
gpu-internal
September 1, 2026 09:57 — with
GitHub Actions
Inactive
aacostadiaz
temporarily deployed
to
gpu-internal
September 1, 2026 09:57 — with
GitHub Actions
Inactive
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
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.

The old package stays in the tree as the reference the new code is measured against. That only holds while the new code cannot reach it: a legacy class you can import is one you can subclass and quietly copy, and then the reference is no longer independent of what it is judging. Two gates, so this stops depending on review attention.
Import contracts.
.importlinterholds five: the v1 packages never importmace,macenever imports them,mace_coredepends on no framework and no sibling, the torch and jax sides never import each other, andmace_torchlayers only import downwards. Each was checked by breaking it and watching it fail on the right file and line.Every layer in the layering contract is optional.
mace_torchhas none ofcli,train,models,nnorkernelsyet, and a contract naming a module that does not exist is simply broken, so the alternative was adding this gate after the imports it exists to prevent.Name check. A file can copy structure without importing anything, so this catches a legacy symbol named in a v1 file at all. It reads the parsed syntax tree, not the text: the list contains
MACE, which is also the project's name and is in nearly every docstring here, so a text search would flag the whole repository and be switched off within a week.getattr(module, "ScaleShiftMACE")is caught;"""The PyTorch stack for MACE v1."""is not. The model-class list is checked against the classes the old code declares, so one added there fails here.Where it runs.
tests/architecturemoves out ofunitinto a newarchitecturejob, so it runs once. That job is the only one installing both trees editable, which the contracts need: import-linter resolves each package on the filesystem, so a missing one reads as a broken contract rather than a missing install. It does not wait onlint, since a formatting failure and an import-direction failure are different answers.Checks run
lint-imports --config .importlinterpytest tests/architecturepytest tests/unit tests/golden -m "not slow" -n autopre-commit run --all-filesgit ls-files mace/mace/is emptyThe runtime half of the guard already exists in
mace_launcher.audit. Its other activation point is thetests/parity/conftest, which PAR-1 (#1572) creates.What to look at
That exactly two things may import both stacks:
mace_launcher, which holds the dispatcher and the guard, andtests/parity/. The launcher's entry is its absence from the contract's source list, which is easy to widen by accident and impossible to notice, so a test pins that list.Closes #1552
Note
Low Risk
Changes are CI and dev-tooling guardrails only; they do not alter training, inference, or user-facing runtime behavior.
Overview
Adds static enforcement so the frozen legacy
macetree stays isolated from the v1 packages underpackages/, complementing the existing runtime guard inmace_launcher.audit.Import contracts (
.importlinter, run vialint-imports): five rules—no cross-import between legacy and v1 (withmace_launcherintentionally outside the v1→legacy sources), framework-freemace_core, nomace_torch↔mace_jaxmixing, and optional downward layering formace_torch.import-linteris added to the dev extra.CI:
tests/architectureis removed from the unit matrix and moved to a dedicatedarchitecturejob that installs both stacks (legacy viasetup-mace, v1 editable), runslint-imports, thenpytest tests/architecture—in parallel with lint, not behind it.New architecture tests: mirror the contracts locally (including pinning that only
mace_core/mace_torch/mace_jaxare forbidden-from-legacy sources) and scan allpackages/**/*.pywith an AST denylist of legacy model/registry/data names so structural ports are caught even without imports.Reviewed by Cursor Bugbot for commit ef56d31. Bugbot is set up for automated code reviews on this repo. Configure here.