Skip to content

feat: register every augmentation - #68

Open
Hendrik-code wants to merge 1 commit into
hm/registry-corefrom
hm/register-transforms
Open

feat: register every augmentation#68
Hendrik-code wants to merge 1 commit into
hm/registry-corefrom
hm/register-transforms

Conversation

@Hendrik-code

Copy link
Copy Markdown
Collaborator

Stacked on #67.

Each of the 51 augmentation classes now carries an @register(...) decorator naming its backend, its GEO/GE/TA group, and the backend-neutral concept it implements. smauglab/transforms/__init__.py imports them all, so importing that package is what populates the registry — that's what registry.load_all() does.

The batchgeneratorsv2 transforms the CPU pipeline composes are third-party and can't be decorated, so smauglab/transforms/cpu/external.py builds their entries by hand. Two things are declared per entry rather than assumed:

  • wrap_random=False for transforms appended directly rather than inside a RandomTransform — they own no application probability, so a config setting p on them is an error.
  • context_params for values nnU-Net supplies at runtime (patch size, rotation range) — a config must not set those.

The registered set and PIPELINE_ORDER now agree exactly: 29 GPU, 22 CPU — checked in both directions. Registration rejects a class missing from the table; a test rejects a table entry that nothing registers.

The if ladders still run and still decide what a config builds. Nothing reads the registry yet except the tests. That's deliberate: this PR is only about the metadata being present and correct.

Two things that had to come along

RandomDomainTransferGPU still took **kwargs, so registering it failed the no-**kwargs-without-forwards_to rule. Removed.

⚠️ #66's commit message claimed "**kwargs is gone from every transform constructor". That was true of every file it touched, but it did not touch domain_transfer.py — I'd reverted that file there to keep the PR focused. I've corrected #66's description.

Its bank path was a hardcoded absolute path into one machine's NAS home directory:

DEFAULT_BANK_PATH = "/DATA/NAS/ongoing_projects/.../domain_transfer_bank.npz"

That is exactly what the registry's external_asset field exists to describe, so it becomes the SMAUGLAB_DOMAIN_BANK environment variable — no baked-in default, and an error message that names the fix. Tests consult the registry entry rather than hardcoding the class name, so a second such transform is covered automatically.

Verification

With SMAUGLAB_DOMAIN_BANK pointing at the bank, all 24 seeded config digests are unchanged, including the domain-transfer one. Without it, that single config skips instead of running — the only behaviour difference, and the point of the change: it previously resolved by accident on exactly one machine and was silently broken for everyone else.

Full suite: 141 passed, 5 skipped, 478 subtests (up from 237 — every registered augmentation now gets a construct-and-run smoke test). ruff and mypy clean.

Each of the 51 augmentation classes now carries an @register(...) decorator naming
its backend, its GEO/GE/TA group and the backend-neutral concept it implements.
smauglab/transforms/__init__.py imports them all, so importing that package is what
populates the registry -- that is what registry.load_all() does.

The batchgeneratorsv2 transforms the CPU pipeline composes are third-party and
cannot be decorated, so smauglab/transforms/cpu/external.py builds their entries
by hand. Two things there are declared per entry rather than assumed:
wrap_random=False for transforms appended directly rather than inside a
RandomTransform (they own no probability, so a config setting `p` is an error),
and context_params for values nnU-Net supplies at runtime.

The registered set and registry.PIPELINE_ORDER now agree exactly, 29 GPU and 22
CPU, and that is checked in both directions: registration rejects a class missing
from the table, and a test rejects a table entry nothing registers.

The `if` ladders still run and still decide what a config builds. Nothing reads
the registry yet except the tests, which is deliberate -- this commit is only
about the metadata being present and correct.

Two things had to come with it:

* RandomDomainTransferGPU still took **kwargs, so registering it failed the
  no-**kwargs-without-forwards_to rule. Removed. The previous commit's message
  claimed **kwargs was gone from every transform constructor; that was true of
  every file it touched, but it did not touch this one.

* Its bank path was a hardcoded absolute path into one machine's NAS home
  directory, which is what external_asset exists to describe. It becomes the
  SMAUGLAB_DOMAIN_BANK environment variable, with no baked-in default and an
  error message that names the fix. Tests consult the registry entry rather than
  hardcoding the class, so a second such transform is covered automatically.

Verified: with SMAUGLAB_DOMAIN_BANK pointing at the bank, all 24 seeded config
digests are unchanged, including the domain-transfer config. Without it that one
config skips rather than running, which is the only behaviour difference and the
point of the change -- it was previously found by accident on exactly one machine.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 21, 2026 06:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants