Repository navigation
Refactor how models and configuration are handled - #86
Conversation
A profiling study varies along two axes that are not the same kind of thing. How a model is run - where the engine puts its output, what the scheduler recorded, how a job is submitted - is behaviour. What is being run - which components exist, on which grids, which files state the parallelism, which logs to read - is data. These two modules are the data half. One rule places every member: the manager reads what the engine and the scheduler wrote, the configuration reads what the model's own configuration files say. So the pair that writes a layout into those files and reads one back out of them lives together here, being inverses over the same text. A control is neither. The same configuration can be profiled from a released tag, from a fork, or from a directory already on disk, and none of those change what the model is or how it is run. The axis a control varies on is how it is retrieved, not which runner uses it: ACCESS-AM3 is driven by Cylc and takes its control from git exactly as the Payu-driven models do, while ACCESS-rAM3 shares its runner and takes one from a directory checked out beforehand. Both runners accept both, so provenance carries no per-runner restriction. A control source records coordinates and fetches nothing, which keeps ssh keys and MOSRS passphrases outside this package, where a prompt cannot hang a notebook waiting for input nobody can give it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ProfilingManager now takes the configuration being profiled and the control it perturbs, and the four model-shaped members - parallel_component, layout_branch_name, layout_config_changes and parse_layout - stop being abstracts for a subclass to answer and become questions for the configuration. _layout_component_names goes with them, into LogSpec.component, which removes the one model-axis member that failed silently: a log it did not name was read against itself with no complaint. The generation loop hoists to the base, since deciding what to generate - enumerate the layouts of each size, skip the ones already held, ask the configuration for a name and its changes - is the same question for every workflow engine. Creating what was decided is each engine's own and is left to _create_experiments, so the Cylc layout work inherits the loop rather than writing it again. PayuManager and CylcRoseManager become concrete. Neither has a model-shaped member left, so ACCESS-ESM1.6 and ACCESS-OM3 stop being subclasses, and CylcRoseManager stops overriding four abstracts purely to raise. ControlSource gains origin and start_point, which is what a runner that fetches the control needs; Payu asks for them and says so plainly when a control states no start point. set_control stays as a deprecated shim over GitControlSource. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
access_models.py held four manager subclasses whose whole content was data: the component tree of one model, the names of its logs, what its layout comes to in its own configuration files. They are now values in access.profiling.models, one module per model, and nothing about them is a class with behaviour of its own. ACCESS-ESM1.6 gains what it never had: which configuration is being profiled. The config repository releases a dozen, and the coupled ones cannot be told apart by their layout - piControl-2.1 and esm-historical-1.3 have the same grids, the same components and the same core counts - so an experiment name derived from a layout alone is one string for two experiments, and that string is a dict key, a git branch, a directory, an archive stem and config.yaml's own experiment key. Names now carry the configuration. AMIP comes with it: the components are optional, so an atmosphere-only configuration builds an atmosphere-only tree, and the submodels a layout is merged into are declared rather than assumed, which keeps a three-component edit from silently deleting the OASIS slot the generator would truncate away. CICEPartitioning holds what a sea ice may be asked about. CICE takes a block size rather than a process grid, and only cartesian and rake form one at all, by searching for a factorisation that suits processor_shape; the other six distributions never do. So the domain, the constraints and the namelist writer all follow from the two namelist strings the control sets, where before they agreed only by hand - ESM1.6 calling its domain one-dimensional because the component is CICE5, which is not the reason, and OM3 writing block sizes derived from a process grid for a release that distributes roundrobin, where that grid means nothing. The 25 km release is square-ice on roundrobin, so the search now chooses its sea ice's rank count and nothing else, and writes it no ice_in. access_models.py stays for one release, re-exporting the models and keeping the four names as factories that build a manager around a configuration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
name, model_type and experiment_prefix were properties over underscored fields, which put _name= in every call that builds a configuration. A dataclass field with a default satisfies the abstract property on its own, so the underscores go and the fields are simply what they are called. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test_access_models.py splits to match src/access/profiling/models: one file per model, plus test_cice.py for the partitioning both sea ice components share. The model tests lose their managers. A configuration answers for its own tree, its own names and its own configuration files, so a test about ACCESS-OM3 builds no manager at all except to run the layout search, which is the one thing the manager still owns. MockPayuManager and MockCylcManager disappear with them: both runners are concrete, so their tests build the real class around a mock configuration. Two expectations move, deliberately. ACCESS-ESM1.6's experiment names now carry the configuration, because the coupled releases are indistinguishable without it. And ACCESS-OM3's 25 km release no longer has a sea ice decomposition to assert: it distributes square-ice blocks roundrobin, which forms no process grid, so the search picks its rank count and writes it no ice_in. What the old tests pinned there was a process grid converted into block sizes for a scheme that reads neither. The tests that are really about a chosen decomposition move to the 100 km development configuration, whose blocks are cartesian with one block row. New ground: AMIP's atmosphere-only tree and single submodel slot, the OASIS slot PRESERVE keeps, two configurations with one layout naming two experiments, which logs each configuration declares, and the three partitioning modes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #86 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 16 23 +7
Lines 1641 1970 +329
==========================================
+ Hits 1641 1970 +329 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ee7349b to
5b2621b
Compare
micaeljtoliveira
left a comment
micaeljtoliveira
left a comment
There was a problem hiding this comment.
A couple of general comments:
- the term "model" is used in a few places, but that's very Climate science specific. A more general term would be "application".
- "configuration" is also very climate science specific. In other fields, like electronic structure, one uses the term "input" (which in climate science has yet a different meaning). Would be good to use a more generic name.
The package has not reached beta, so a bridge for callers of the old API is weight without a reader. set_control goes, and with it the only use of warnings in payu_manager; access_models.py goes entirely, its 46 re-exports already being available from access.profiling.models and its four factories having had one caller, a test asserting that they warn. One test was patching pathlib.Path through access_models' namespace, which worked only because Path is a single global object and would have broken the moment that module went. It now patches pathlib directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It describes an experiment before any engine has touched it, and ProfilingExperiment describes the same thing afterwards - down to carrying the same ComponentLayout, which experiment.py already imported for it. Having the two halves in different modules said that the plan was part of the configuration contract, which it never was: no configuration builds one, reads one, or mentions one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The module meant to state what every application has in common held two log
locators that knew Payu's archive layout and the UM's environment file, and two
classes that knew Payu and Rose outright. Anything that names a runner now lives
in a module named for that runner: payu_configuration.py takes PayuConfiguration
and payu_model_stdout, rose_configuration.py takes RoseSuiteConfiguration, and
um_stdout joins the rest of the UM's knowledge in um_parser.py. The YAMLParser
import leaves with the locators, which were its only users.
The two new modules are deliberately not folded into the managers. payu_manager
reaches for the experiment generator and runner, and so for Payu itself;
describing an application should not import a job-submission stack, nor point
the data axis at the runner axis. The graph now reads
models/esm16 -> payu_configuration -> configuration -> experiment -> parser
with no module under models/ importing a manager.
log_at stays: it closes over an arbitrary relative path and knows no runner at
all. Its callers supply the specificity, which is where it belongs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"Model" is a climate-science word, and nothing in the generic layer is about climate: a manager runs a thing, reads what it wrote, and plots how it scaled. ModelConfiguration becomes Application, configuration.py becomes application.py, and the managers take and expose it under that name. The ACCESS-specific layer keeps its own vocabulary. models/, ESM16Configuration and OM3Configuration are not renamed: there the subject really is a climate model, and "configuration" is what the upstream repositories themselves call a released setup. PayuConfiguration and RoseSuiteConfiguration keep their names too, now that they live in modules of their own. Untouched throughout: model_type, submodels, config.yaml and its model key, and the experiment generator's and runner's dict keys. Those are other people's words and renaming them would break the call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
manager.py and control.py already imported nothing runner- or model-specific; only their prose gave the game away, naming ACCESS-ESM1.6, ACCESS-OM3, Payu, Cylc, rosie and MOSRS to illustrate points that hold without them. The examples are now stated in the terms the code is actually written in - a coupled framework's summary log, a component sharing a range of cores, a credential-protected repository - so a reader of the generic layer is not asked to know a particular climate model to follow it. One of them was a layering slip rather than a wording choice: the base class documented a subclass's limitation, raising NotImplementedError "as Cylc Rose ones do not". It now states the condition instead of naming who meets it. The Application ABC and the managers' prose follow the rename: an application has setups, not configurations, and reads its own input files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both taken, in 50633c0 and de3ee11, though I drew the line between them rather than renaming everywhere. "model" → "application". The generic layer is now written in those terms throughout: "configuration". I kept it where the context makes it the right word and dropped it where it does not. So the rule is roughly: generic modules speak of applications and runners; the ACCESS-specific modules speak of models and configurations. If you would rather the leaves moved too, that is a small follow-up — it is five class names. Deliberately untouched: All six inline comments are answered above. Summary of the five commits:
500 tests pass, coverage is 100% on statements and branches, and all eight checks are green. Two things I did beyond the literal requests, both noted in the threads above: Posted by Claude on behalf of @micaeljtoliveira. |
| OM3_25KM_CICE6: CICEPartitioning = CICEPartitioning( | ||
| grid=OM3_25KM_GRID.shape, | ||
| distribution_type="roundrobin", | ||
| processor_shape="square-ice", | ||
| nghost=OM3_CICE6_GHOST_WIDTH, | ||
| ) |
There was a problem hiding this comment.
This information, and probably a few more, could be inferred from the config files instead of being hardcoded. This would require cloning the git repo before calling the experiment generator, but making sure it's clone to the same location as the experiment generator would.
There was a problem hiding this comment.
Agreed on the substance, and I have opened #87 rather than fold it into this PR, which is already eleven commits.
Two things I found while scoping it that are worth recording.
The mechanics are cheaper than they look. access.config already ships FortranNMLParser, MOM6InputParser, NUOPCParser and YAMLParser, and OM3Configuration.parse_layout already reads nuopc.runconfig through NUOPCParser. And the clone location is well defined: experiment_generator.base_experiment resolves self.directory = (self.test_path / self.repo_dir).resolve(), so cloning there first is exactly what you describe and the generator would reuse it. Beyond OM3_25KM_CICE6 the same applies to MOM6's extents (MOM_input's NIGLOBAL/NJGLOBAL), the released core counts and shared-core offsets (PELAYOUT_attributes), ESM16_CICE5 (ice/cice_in.nml), and the submodel order and length config_changes has to preserve (config.yaml).
The design consequence is the part I would not decide on my own. An Application is currently a value: ESM16_PI_CONTROL can be built, compared and reasoned about with no filesystem and no network, and the same value can be profiled against several releases precisely because it does not know which one it describes - that is use-case 5, "same model, different commit". Deriving its fields from a control reverses that: the configuration could not exist until something had been fetched. It also cuts against what ControlSource is documented to be, a record of coordinates that deliberately fetches nothing so that a credentials prompt cannot hang a notebook kernel.
So in #87 I have written up a middle ground as well, which you may or may not want. Keep the configuration a value, and add a verification step instead of a derivation: given a checked-out control, read its files and report where the shipped constants disagree with it. That removes the actual risk here - the package quietly describing a configuration it no longer matches, which surfaces only as a layout that will not run - without making every configuration depend on a clone. Full derivation could then be an opt-in constructor that builds a configuration from a directory.
Happy to go straight to full derivation if you would rather; it is mostly a question of whether a configuration should be able to exist without a control.
Posted by Claude on behalf of @micaeljtoliveira.
Pylance reports 111 errors on this branch against 64 on main, and 47 of the difference is ours. None of it was caught, because nothing type-checks the repo: CI runs ruff and pytest only. Three were real, not merely unsound: - PayuManager._create_experiments guarded `self.control.start_point`, which dereferences the control before checking it. A manager with no control raised AttributeError from the guard itself, leaving the ValueError underneath it unreachable. generate_scaling_experiments refuses a missing control first, but the hook is callable on its own and `control` is a settable property, so the check belongs where the attribute is read. - ESM16Configuration.config_changes reached through a missing decomposition. parse_layout states cores and not grids, and the two are documented as inverses, so the obvious round trip died on an AttributeError although the docstring promises a ValueError. _process_grid now says so properly. - PayuManager and CylcRoseManager narrowed _create_experiments away from the base's **runner_options, which generate_scaling_experiments forwards verbatim. control_options= therefore worked on one runner and raised TypeError on the other - masked today only because the Cylc hook raises NotImplementedError before it can be noticed. The rest is the type system catching what the design left implicit. The abstract properties for name, model_type, experiment_prefix and start_point are plain attribute declarations now, since every implementation satisfies them with a dataclass field; a subclass that forgets one is no longer refused by ABCMeta, which is the cost of saying what the subclasses actually do. ProfilingManager became generic over its application, which is what the two managers' __init__ overrides were simulating by declaring a narrow type and forwarding it to a wide one - both overrides are gone, since the type parameter does their whole job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The other 47 of the 64 Pylance reports on main, none of which this branch caused. Two were real bugs. plot_scaling_metrics fitted its efficiency axis to the wrong data. max_eff was initialised inside the loop over datasets, so after the loop it held the maximum of the last one alone and ax2.set_ylim clipped every earlier dataset's curve off the plot. The comment two lines below it exists to stop exactly that - "values above 100% get silently dropped" - so the bug defeated its own guard. The same loop leaked `stat` to the ideal lines and the table title, which then described the last dataset while the table header described the first; both read stats[0] explicitly now. The existing regression test passed a single dataset, which is why none of this showed, so there is a two-dataset test as well. UMProfilingParser checked its header and its footer for a match and then used the combined pattern unchecked. A log holding both markers in the wrong order - truncated, or two logs concatenated - therefore raised AttributeError where the docstring promises ValueError. The largest group was neither a bug nor noise but an undeclared contract: the parsers key one dict by both str and ProfilingMetric, which the module docstring explains at length and no annotation stated, so every read and write of one was reported. Naming it ProfilingData and annotating six declarations settles eleven of them and moves the contract into the type system. What is left is tests reaching through values that are optional in general and present in every case they build. Rather than scatter assertions, tests/conftest.py gains four narrowing helpers, so a broken assumption now reads as a failed assertion naming the layout rather than an AttributeError. Four tests assert that an error is raised for a deliberately wrong argument; there the type complaint is the point, so each carries an ignore saying so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ESM16_CICE5_NX_GLOBAL and ESM16_CICE5_NY_GLOBAL were kept for callers written before CICEPartitioning held the grid. Nothing outside this branch ever saw either name, so there is no caller to keep working, and the package has not reached beta. The tests read the extent off the partitioning instead, which is the one place that states it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
No description provided.