Skip to content

Refactor OceanConfig/Ocean to the Builder pattern#1343

Open
mcgibbon wants to merge 2 commits into
mainfrom
refactor/ocean-builder-pattern
Open

Refactor OceanConfig/Ocean to the Builder pattern#1343
mcgibbon wants to merge 2 commits into
mainfrom
refactor/ocean-builder-pattern

Conversation

@mcgibbon

@mcgibbon mcgibbon commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Corrects the fme/core/ocean.py Builder-pattern violation (the calibration example the pre-review skill cites) under the ace obeys its Builder pattern pass burn-down. OceanConfig.build() returned Ocean(config=self, ...), and Ocean.__init__ read raw config fields and built its own Prescriber (rule 1 + rule 2 violations for a non-leaf config).

OceanConfig.build() now constructs the Prescriber and the surface-temperature model, and passes those built collaborators into Ocean. Ocean no longer receives OceanConfig and reads no config fields. Following the merged corrector template (#1313), a private _build() on the config does the assembly and the prescribed/slab dispatch that Ocean.__call__ did via a self.type string becomes two small callable SurfaceTemperature objects, each bundling the field names/operators it needs.

Judgment calls (OceanConfig is non-leaf via slab):

  • The remaining scalars Ocean needs (surface_temperature_name, ocean_fraction_name) are passed as explicit constructor arguments rather than packed into a non-dacite leaf dataclass — they are just field names, so a params bag would be boilerplate, not clearer separation. (Ocean.prescriber stays public: production step code calls it directly.)
  • Added OceanConfig.is_slab as the public accessor the coupled-stepper validation (and the downstream step-config wave) reads instead of the raw slab field; adopted at the one coupled-stepper .slab is not None read.

The step family (fme/core/step/*, fme/ace/step/fcn3.py, coupled steppers) still reads self._config.ocean.* and builds Ocean itself — those are the separate, downstream step-level fixes; this PR lands the Ocean exemplar they depend on.

Changes:

  • fme.core.ocean: OceanConfig.build/_build construct the Prescriber and a SurfaceTemperature (new PrescribedSurfaceTemperature/SlabSurfaceTemperature callables + SurfaceTemperature protocol); Ocean.__init__ takes built collaborators, not the config; add OceanConfig.is_slab.

  • fme.coupled.stepper: coupled-emulation validation reads OceanConfig.is_slab.

  • fme.core.test_ocean: construct Ocean via config.build(...).

  • Tests added (existing ocean unit tests updated to build via config.build(); behavior-preserving)

  • If dependencies changed, "deps only" image rebuilt and "latest_deps_only_image.txt" file updated

OceanConfig.build() now constructs the Prescriber and the surface-temperature
model (prescribed or slab) and passes those built collaborators into Ocean,
which no longer receives OceanConfig or builds any sub-object (Builder pattern
rule 2). The prescribed/slab dispatch that Ocean.__call__ did via a self.type
string becomes two small callable SurfaceTemperature objects, following the
merged corrector template (#1313): a private _build() on the config assembles
the collaborators; the impl is thin and reads no config fields.

Add OceanConfig.is_slab so the coupled-stepper validation (and the downstream
step-config wave) reads a public accessor instead of the raw slab field.

Behavior-preserving; ocean unit tests build via config.build().
@mcgibbon

mcgibbon commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Claude: Independent pre-review (worktree-isolated, no authoring context). Ran the ace pre-review skill's audit passes against the diff and the full test set. Verdict below — no code changes applied (nothing needed fixing).

Audit passes

  • 1. Silent-failure. No new dacite unions/defaults. The prescribed/slab dispatch moved from a self.type string to if self.slab is None in _build — the same two-state dispatch. The old else: raise NotImplementedError(f"Ocean type={self.type}") was already unreachable (type was only ever set to "prescribed"/"slab"); dropping it removes dead code, not a live guard.
  • 2. Behavior-change on modified paths. Verified line-by-line equivalence: Prescriber args identical (mask_value=1, interpolate); prescribed temp = target_data[sst]; slab tendency formula and * timestep.total_seconds() unchanged; __call__ still returns prescriber(target_data, gen_data, {sst_name: next_temp}). is_slab == slab is not None (identical boolean at the coupled-stepper site).
  • 3. Differential-test. test_ocean_prescribed/test_ocean_slab use distinct per-cell values and exercise both callables; a no-op'd surface-temperature would fail them. Re-wired to build via config.build(...) — same assertions.
  • 4. Sibling/mirror. The one external OceanConfig.slab read (coupled stepper) is the only site needing is_slab; grep confirms no other external .slab read. Step-config reads of self._config.ocean.* are the explicitly-scoped downstream wave, not siblings of this change.
  • 5. Config back-compat. No OceanConfig/SlabOceanConfig field added, removed, or renamed; build() signature unchanged. All prior YAML parses identically; nothing checkpoint-persisted changed.
  • 6. Split. 3 files, +115/-39. No split warranted.
  • 7. Abstract-or-final / inheritance depth. SurfaceTemperature is a Protocol; the two impls satisfy it structurally (no inheritance). Ocean has no subclass. No in-repo inheritance introduced.
  • 8. AI-agent artifacts. No Any returns, no isinstance+type: ignore, no in-function imports, descriptive identifiers, docstrings updated to the new args. PrescribedSurfaceTemperature ignoring input_data/gen_data is the uniform-callable interface, not dead params.
  • 9. Builder pattern. The impl (Ocean) no longer receives OceanConfig and reads no config fields — it takes built collaborators (Prescriber, SurfaceTemperature) plus two scalar names; build()/_build() does the assembly and the prescribed/slab dispatch. This is a cleaner application of the pattern than the cited corrector template, whose impl still holds self._config. No new violations introduced. The pre-existing config-field reads in coupled/stepper.py (surface_temperature_name, ocean_fraction_name) and the step configs' self._config.ocean.* are untouched and allowed as pre-existing (and are the described downstream wave).

Verification (env slot on PR HEAD 8e0e2b8cb): fme/core/test_ocean.py (3) + fme/core/step/, fme/ace/step/, fme/coupled/test_stepper.py, fme/ace/inference/test_evaluator.py (233) all pass. ruff check clean; pre-commit run mypy passes on all three changed files (the SurfaceTemperature Protocol assignment type-checks).

On the three judgment calls — I agree with all three:

  1. Passing surface_temperature_name/ocean_fraction_name as explicit args rather than a leaf params-bag is right: surface_temperature_name is genuinely used by Ocean.__call__ (the prescriber write key) and both are read externally on the Ocean instance by test_evaluator.py, so they must stay as attributes; a 2-string dataclass would be boilerplate, not separation. Per the skill's non-leaf exception this is a defensible call, not a violation.
  2. Ocean.prescriber must stay public — single_module/radiation/secondary_module/fcn3 all call self.ocean.prescriber(...) directly (pre-existing).
  3. On _build reading self.slab.q_flux_name/.mixed_layer_depth_name to construct SlabSurfaceTemperature: this is rule-2 sanctioned builder construction (the builder assembling a sub-object from its sub-config's fields), not decision-logic field-snooping, so it is not a violation. A marginally cleaner alternative would delegate to a SlabOceanConfig.build_...() method (mirroring its existing .names property), but that is optional polish, not required.

Minor optional note (non-blocking): with no DatasetInfo to unpack, the build()/_build() split is thinner here than in the corrector template — _build has a single caller. It does cleanly separate the in_names/out_names validation from timestep-only assembly, and keeping it matches the template for burn-down consistency, so leaving it as-is is fine.

Ready for review. No structural issues, no blocking findings, behavior-preserving.

@mcgibbon mcgibbon marked this pull request as ready for review July 7, 2026 14:18
Comment thread fme/core/ocean.py
Comment on lines +133 to +142
surface_temperature = PrescribedSurfaceTemperature(
self.surface_temperature_name
)
else:
surface_temperature = SlabSurfaceTemperature(
surface_temperature_name=self.surface_temperature_name,
q_flux_name=self.slab.q_flux_name,
mixed_layer_depth_name=self.slab.mixed_layer_depth_name,
timestep=timestep,
)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note for reviewers: We could consider refactoring to have these two classes be explicitly defined config classes in this configuration, but the scope of this PR is explicitly not to change configuration or behavior.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Claude: Agreed — kept promoting these to explicit config classes out of scope (no configuration/behavior change). Separately, per your other note I made the dataclasses frozen where it makes sense: both value objects (PrescribedSurfaceTemperature, SlabSurfaceTemperature) and the never-mutated OceanConfig/SlabOceanConfig are now frozen=True (29e3672). Full test suite still passes.

Comment thread fme/core/ocean.py Outdated
return [self.mixed_layer_depth_name, self.q_flux_name]


class SurfaceTemperature(Protocol):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

nit: Because we directly control all implementations of this class locally, make this an ABC inherited by its implementors, instead of a protocol.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Claude: Done in 29e3672SurfaceTemperature is now an abc.ABC with an abstract __call__, inherited by PrescribedSurfaceTemperature and SlabSurfaceTemperature (no longer a structural Protocol).

- SurfaceTemperature is now an abc.ABC inherited by PrescribedSurfaceTemperature
  and SlabSurfaceTemperature, rather than a structural Protocol, since all
  implementations are controlled locally.
- Freeze the value-object dataclasses (PrescribedSurfaceTemperature,
  SlabSurfaceTemperature) and the never-mutated config dataclasses
  (OceanConfig, SlabOceanConfig) with frozen=True.
@mcgibbon mcgibbon requested a review from spencerkclark July 7, 2026 17:56

@spencerkclark spencerkclark left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the way this simplifies the Ocean class is an improvement. I'm good with these changes, especially if it makes things more consistent with patterns used elsewhere in the code.

Comment thread fme/core/ocean.py


@dataclasses.dataclass(frozen=True)
class SlabSurfaceTemperature(SurfaceTemperature):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: I have a slight preference for calling this SlabOceanSurfaceTemperature

Suggested change
class SlabSurfaceTemperature(SurfaceTemperature):
class SlabOceanSurfaceTemperature(SurfaceTemperature):

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