Shared configs and framework fixes - #686
Open
xylar wants to merge 19 commits into
Open
Conversation
xylar
added
clean-up
framework
xylar
force-pushed
the
share-configs-and-framework-fixes
branch
2 times, most recently
from
August 8, 2026 10:07
9bfe276 to
9fd808c
Compare
xylar
marked this pull request as ready for review
August 8, 2026 10:42
Collaborator
Author
TestingPolaris
|
Collaborator
Author
|
I will also test this with the full unified mesh workflow and will report any issues that arise, but this can be reviewed and merged without waiting for that testing. |
The p-star initialization path bypassed init_vertical_coord/add_1d_grid, so pstar_init.nc lacked the 1D reference vertical coordinate variables (refBottomDepth, refZMid, refTopDepth, refInterfaces). MPAS-Ocean requires refBottomDepth as an input on its mesh stream. Reuse the existing add_1d_grid() in the base PStarInitStep output assembly so every p-star init step produces these fields, and expose a shared REF_COORD_VARS constant for downstream routing/exclusion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The base p-star init now emits refBottomDepth/refZMid/refTopDepth/ refInterfaces for MPAS-Ocean. Omega uses RefPseudoThickness instead and ignores these, so drop them from the Omega initial state file to keep its output minimal and model-appropriate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Assert the base PStarInitStep output contains refBottomDepth and its companions, verify they are kept for MPAS-Ocean but dropped for Omega in write_initial_state_dataset, and guard that REF_COORD_VARS stays in sync with add_1d_grid. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
pyremap's Remapper defaults to use_tmp=True, which puts the SCRIP files and the MOAB .h5m files in a TemporaryDirectory() under /tmp. On Chrysalis (and many other HPC machines) /tmp is a node-local disk, so the MPI tasks running mbtempest on other nodes cannot see the files and abort during the parallel read. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The final savefig used bbox_inches='tight' on a fixed-aspect cartopy GeoAxes, which shrinks the map axes so only part of the domain is drawn -- Antarctica was cut off the bottom of every global lat-lon plot. This is the same failure mode fixed for plot_global_mpas_field in "Fix half-globe clipping in plot_global_mpas_field"; that fix was never propagated to plot_global_lat_lon_field. The fix there was to let constrained_layout manage the margins, but that does not work here: this function attaches its colorbar with inset_axes anchored outside the axes, which constrained_layout does not account for, so the colorbar falls off the canvas entirely. Reserve the margins explicitly instead, which keeps the full domain, the colorbar and its tick labels. Affects all callers of plot_global_lat_lon_field: the woa23 and jra55 hydrography/forcing viz, the e3sm/init combined-topography viz and the unified base-mesh viz. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cartopy's Gridliner draws its own tick labels and constrained_layout does not measure them, so the labels that stick out furthest were clipped at the figure edge. On Robinson the 30-degree labels sit left of the 60-degree ones, so "30N"/"30S" rendered as bare "N"/"S" while "60N"/"60S" survived. Reserve the horizontal margin explicitly on the layout engine. Drawing the figure once before saving, so the labels exist for a second layout pass, does not work -- constrained_layout still does not account for them. Companion to "Fix domain clipping in plot_global_lat_lon_field"; affects every caller of plot_global_mpas_field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A get_*_steps() helper is called once per consumer, and get_or_create_shared_step already makes that safe for the steps. The config those steps use had no equivalent, so a helper that built it unconditionally handed the second caller a *different* config object at the same filepath while the shared steps -- created on the first call -- went on using the first one. Both failure modes are bad. Quietly, options set on what the second caller was handed reach nothing. Loudly, passing it to Task.set_shared_config raises, because add_config refuses a different config at a path that already has one. So Component gains get_or_create_shared_config, the companion to get_or_create_shared_step, taking a setup callback that runs only when the config is really created. It registers the config immediately, which is what lets the second caller find it. _get_target_topo_steps was building its config unconditionally and now goes through it. Eight other modules already hand-roll the same guard, which is why the gap was easy to miss; converting those is left for later, since they are correct as they stand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three tests on the helper itself: that it creates and registers, that it returns the same object on a second call, and that the setup callback runs only once -- otherwise a second caller would re-add its packages on top of options the first caller's steps had already been given. Three more that the real helpers hand back the config their steps are using, for woa23, jra55 and the lat-lon combined topography. Each was checked against the unfixed code: reverting a helper fails its test on the identity assertion, so the tests cover the bug rather than just the new method. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The shared-steps section of the developer guide gains the companion method and, more usefully, why building a shared config unconditionally is a bug even though it looks harmless: the second caller gets a different object at the same path while the steps keep the first, so its options reach nothing and registering it raises. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step.add_dependency was the odd one out among Polaris' registration methods. Component.add_step and Component.add_config both raise only when a *different* object is registered at the same key and treat re-registering the same object as a no-op; add_dependency raised on any repeat of the name. That punishes exactly the pattern shared steps are built around. A get_*_steps() helper is called once per consumer, so a dependency it wires outside a step constructor -- a restart chain, whose links only the helper knows -- was wired again on the second call and raised. The workaround is for the helper to track whether each call is the one that created the step, which is bookkeeping the framework should not be asking for. So the same step under the same name now returns early. Early, not just past the raise: the rest of the method calls add_output_file() and add_input_file(), neither of which de-duplicates. The error worth keeping is the name collision between two different steps, which is what the `name` argument exists to resolve, and that one still raises -- with a message that now says which case it is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three tests: that add_dependency wires the pickle output and input that carry a dependency, that repeating it changes nothing -- checked all the way down to the output and input lists, since those are what would grow if the method fell through rather than returning -- and that a different step under a name already in use still raises. The second and third fail against the previous behaviour, so they cover the change rather than just the method. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Wiring inside the step constructor is the pattern to reach for, since get_or_create_shared_step() passes constructor arguments only when it really creates the step. Where the helper is the only thing that knows the link, add_dependency() may now simply be called again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seven helpers still hand-rolled the guard this method replaces:
if filepath in component.configs:
return component.configs[filepath]
They were correct, which is why the helper that was missing it went
unnoticed until it handed a second caller a config its own shared steps
were not using. Having one way to do this is what stops that recurring,
so the four unified-mesh helpers, topo/cull, topo/remap and mesh/base/add
now go through the method too.
That needed the method to change shape. Three of the seven do not build
a config themselves; they delegate to a builder that creates and returns
one -- get_unified_mesh_config and get_sizing_field_config. A callback
handed a config to populate cannot express those without duplicating
what the builders do or changing their signatures, so the callback now
*returns* the config instead. An existing builder can be passed
straight through, and the method's job reads as what it is: caching, not
construction.
Because a builder could return a config with any filepath, one that does
not match the key is now an error -- it would be registered under a path
it does not know about, which is the same silent mismatch this method
exists to prevent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The "sets up only once" test becomes "builds only once" against the new callback, and gains an assertion that the second call returns the same object rather than only the same contents. A new test covers the filepath check: a builder that ignores the filepath raises, and nothing is registered under the key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The example builds and returns a config rather than filling one in, and the text notes that an existing builder can be passed straight through, along with the filepath requirement that makes that safe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A get_*_steps() helper suggests one symlink name per step, chosen for
consumers whose tasks live elsewhere in the tree, and tasks forward those
names verbatim:
for symlink, step in steps.items():
self.add_step(step, symlink=symlink)
Four tasks -- CullTopoTask, RemapTopoTask, Woa23 and Jra55 -- live in the
directory that holds those very steps, so the symlink lands next to what
it points to: cull_mask beside mask, woa23_viz beside viz. 105 of the
3074 step symlinks in the tree are like that. They are noise, and a
second name for a directory already in view is worse than no name.
add_step now drops those rather than asking every such task to filter
the names itself, which is the same mistake waiting to be made a fifth
time. Paths are compared in full, since two components can have the
same subdirectory layout and a step from another component is somewhere
else on disk however its subdirectory reads.
A symlink to a step nested *deeper* under the task is kept -- the 40
like viz_remapped_unsmoothed_topo -> unsmoothed/viz. Surfacing a step
from further down under a descriptive name is what symlinks are for.
Symlinks are navigational only: step_symlinks is read in one place,
_setup_step, and the run machinery keys off step.name. Nothing about
what runs changes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five tests on add_step: a symlink to a step elsewhere is kept, one that would sit beside its step is dropped, one to a step nested deeper is kept, a step from another component with a coincidentally matching subdirectory keeps its symlink, and the degenerate case where the symlink would name the step itself is dropped. Two of the five fail against the previous behaviour. The two unified-topo tests asserted that every suggested name became a symlink. Their point -- that the task uses the factory's names rather than inventing its own -- is unchanged, so they now say that with the side-by-side ones excluded, and assert that the excluded set is not empty so the check cannot pass vacuously. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The shared-step section of the tasks guide now says that a symlink landing in the directory that already holds the step is dropped, why a task hits that at all -- it passes through names meant for consumers elsewhere -- and that a symlink to a step nested deeper is kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Component._read_cached_files() asks importlib.resources for cached_files.json under polaris.<name>, but a component name need not correspond to an importable package. Only FileNotFoundError was caught, so anything else escaped from Component.__init__. Two cases got through. A name that is not an importable module at all raises ModuleNotFoundError on every Python version. A name that resolves to a plain module rather than a package raises TypeError on Python 3.11, where importlib.resources.files() rejects a non-package anchor; Python 3.12 dropped that check and returns the containing directory instead, so the missing file was caught as before and the failure only showed up under 3.11. Catch all three, since each one means the same thing: there is no cached_files.json to read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xylar
force-pushed
the
share-configs-and-framework-fixes
branch
from
August 8, 2026 11:51
9fd808c to
721a10a
Compare
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
1 participant
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.
This branch collects framework fixes that were developed while working on realistic global ocean initialization but that stand on their own. No new tasks are added and no existing task's results change, apart from the plotting and p-star output fixes noted below.
Shared config options
Polaris lets several tasks share a single step (via
Component.get_or_create_shared_step()) and a single set of config options. The helpers that build shared steps are called once per consumer, but the code that built the config for those steps ran unconditionally on every call. The second caller therefore got a brand-new config object at a path where a different config was already registered: options set on it reached nothing, and handing it toTask.set_shared_config()raised an error. Several places worked around this with an ad-hoc "is it already incomponent.configs?" check, and others simply didn't.Component.get_or_create_shared_config(), the config-level companion to the existingget_or_create_shared_step(); it returns the registered config if one exists and otherwise calls acreatecallback to build it.filepathmatches the path it is registered under.Step.add_dependency()now treats adding the same step under the same name as a no-op, so aget_*_steps()helper that wires dependencies outside a step constructor is safe to call once per consumer. Adding a different step under a name already in use is still an error, and the error message says so.organization/steps.md) and the API listing.1D reference vertical coordinate in p-star initial conditions
PStarInitStepnow writes the 1D reference vertical coordinate (refTopDepth,refZMid,refBottomDepth,refInterfaces) to its output, since MPAS-Ocean requiresrefBottomDepthas an input.polaris.ocean.vertical.grid_1d.REF_COORD_VARS, so the writer and the Omega filter cannot drift apart.Other fixes
MappingFileStepno longer lets pyremap write its SCRIP and MOAB files under/tmp. That directory is node-local on many HPC machines, while the MPI tasks building a mapping file are typically spread over several nodes, so the files have to be in the step's work directory on a shared filesystem.plot_global_lat_lon_field()no longer saves withbbox_inches='tight', which could shrink the fixed-aspect map axes so that only part of the domain was drawn (Antarctica was being clipped off the bottom). It uses explicit margins instead — the same fix already applied toplot_global_mpas_field(), which had never been propagated here.plot_global_mpas_field()reserves a margin for cartopy's gridline labels, whichconstrained_layoutdoes not measure and which were being clipped at the figure edge.Testing
filepathcheck, the repeated-dependency no-op, the 1D reference coordinate in p-star output and its removal for Omega, and theMappingFileSteptemporary-file setting.Checklist
api.md) has any new or modified class, method and/or functions listedTestingcomment in the PR documents testing used to verify the changes