Add SimBoard-compatible www inference via a new [simboard] config section#841
www inference via a new [simboard] config section#841Conversation
forsyth2
left a comment
forsyth2
left a comment
There was a problem hiding this comment.
@tomvothecoder I had Copilot draft an initial diff for resolving #832, based on notes from our meeting today. I've added some review comments below, including 3 design decision questions for you. Thanks!
|
|
||
| * If ``reference_data_path`` (the path to the reference data) is undefined, assume it is the ``diagnostics_base_path`` from Mache plus ``/observations/Atm/climatology/``. (So, it is important to change this for model-vs-model runs). | ||
|
|
||
| For the top-level ``www`` parameter: |
There was a problem hiding this comment.
Note: these doc changes will be overwritten by the changes coming soon in #839. That PR should be updated if this PR merges first.
| www = string | ||
| # Leave blank to infer `<web_portal base_path>/simboard/<simboard_type>/` | ||
| # when `infer_path_parameters = True` | ||
| www = string(default="") |
There was a problem hiding this comment.
Design decision: How do we want users to specify they want to use the SimBoard paths? Is leaving www blank a sufficient method?
Or should we have an explicit "use_simboard" parameter? In some sense, the new simboard_type parameter is such a parameter.
There was a problem hiding this comment.
I think change makes sense, where www is an optional parameter with a default blank value that gets inferred to SimBoard paths. We'll just need to add a clear logger message and update zppy docs to communicate this new behavior. This would not be a breaking change, but users should be aware of it if they want to start defaulting to SimBoard paths to display their diagnostics on SimBoard.
@chengzhuzhang Thoughts?
| # Reservation -- if you have access to a node reservation, specify it with this parameter. | ||
| reservation = string(default="") | ||
| # Which SimBoard namespace to use when inferring `www` | ||
| simboard_type = option("prod", "dev", default="prod") |
There was a problem hiding this comment.
We should add a "none" option too.
There was a problem hiding this comment.
We are going to work with the SimBoard team to determine the values for simboard_type. I'll follow up here after we do this.
There was a problem hiding this comment.
Alternative based on Jill's preference for generic naming: simulation_type, run_type, diag_type, etc.
| simboard_type = config["default"]["simboard_type"] | ||
| web_portal_base_path = machine_info.config.get("web_portal", "base_path") | ||
| config["default"]["www"] = ( | ||
| f"{web_portal_base_path}/simboard/{simboard_type}/" |
There was a problem hiding this comment.
Design decision: Is this the web portal base path we want? (Remember, the URL is deterministic based on the base path).
Do we want to include some sort of $USER directory? NERSC has the interesting first_letter/full_username structure. I believe Compy has stricter permissions and won't allow new subdirectories outside the user directories unless explicitly requested -- I suppose we could have a simboard directory at the same level as all the user subdirs.
There was a problem hiding this comment.
This looks right to me. @chengzhuzhang can provide comment too.
The www structure could look like {web_portal_base_path}/simboard/{simboard_type}/{hpc_username}/{case_name}. Example: {web_portal_base_path}/simboard/production/ac.tvo/v3.LR.historical.
We are going to work with the SimBoard team to determine the values for simboard_type.
There was a problem hiding this comment.
yeah, we can decide later with the user group on these details. I personally prefer more generic names:
{web_portal_base_path}/simboard - >{web_portal_base_path}/diags_archive
simboard_type -> simulation_type
There was a problem hiding this comment.
I'm happy with generic names too. It makes sense if we don't want to tie parameters and directory names to only one tool like SimBoard. This makes it clear that the output is tool-agnostic.
PACE employs generic naming for its output directories, including performance_archive, OLD_PERF, etc.
There was a problem hiding this comment.
we can decide later with the user group on these details
Do you have an idea of when that would be? I just want to make sure I can get this merged by your deadline. Or is this prototype PR good enough to use for now?
There was a problem hiding this comment.
@forsyth2 We have a SimBoard meeting next Tues and will discuss this. The deadline for diagnostics is flexible and can be pushed a few weeks if needed.
| For the top-level ``www`` parameter: | ||
|
|
||
| * If ``www`` is undefined and ``infer_path_parameters = True``, assume it is ``<web_portal base_path>/simboard/<simboard_type>/``. | ||
| * If ``www`` is undefined and ``infer_path_parameters = False``, ``zppy`` will raise an error. |
There was a problem hiding this comment.
Design decision: should we do the same for output?
Note that output isn't nearly as standardized. Web portal base path works because there's exactly one path prefix that gets posted online on the web portal base url. It sounds like SimBoard is most interested in the URL though, considering #836 added an explicit diagnostics_url line in the new provenance settings file.
There was a problem hiding this comment.
Can you clarify what output is?
I think SimBoard will only need to scan the deterministic path prefix and extract the web portal base url (diagnostics_url).
There was a problem hiding this comment.
output is sort of the staging area. It contains the output of NCO and e3sm_to_cmip for instance. It also contains everything about the zppy run -- the bash scripts, settings, job outputs, etc.
www is the file path that (if actually on a proper web server path) will be served to a web URL. Visual output (e.g., from e3sm_diags, mpas_analysis) goes here.
So, if SimBoard only cares about URLs, www is the only one that matters. If it also cares about data file paths, output would be useful.
There was a problem hiding this comment.
Got it, thanks for clarifying. I'll discuss with Jill and other SimBoard team members to see if output is useful or if www for the diagnostics viewer is all that is needed.
|
@forsyth2 Thanks for opening this PR so quickly. I replied to some of your comments above and tagged Jill for input. |
|
@chengzhuzhang and @forsyth2 here is my proposed design based on the SimBoard team meeting. Zppy diagnostic directory structureI think zppy should publish SimBoard-compatible diagnostics under one bounded archive root: This gives SimBoard one narrow discovery target, avoids broad scans across unrelated web content, and keeps production and development output separated under one common root. Directory layout would be used for discovery only; Zppy configFor config, I think this should be explicit and opt-in via a If Expected behavior:
[default]
www = # Diagnostic web output root. If empty and simboard.enabled=True, infer a SimBoard-compatible archive path from mache.
[simboard]
enabled = False # Opt in to SimBoard-compatible publishing behavior.
simulation_type = development # Diagnostic classification for SimBoard publishing: production | development.Let me know what you think. We can discuss more at next week's EZ Meeting too. |
|
Hi @tomvothecoder, thanks for the design. Checking that each of my design decisions above have been addressed: Design decision 1
Addressed by:
This makes sense, but adding a new In short, this can be implemented, but it's a little bit more overhead. It sounds like that's worth it for clarity to users though. Design decision 2 simboard_type = config["default"]["simboard_type"]
web_portal_base_path = machine_info.config.get("web_portal", "base_path")
config["default"]["www"] = (
f"{web_portal_base_path}/simboard/{simboard_type}/"
Addressed by:
So, it looks like we just want to change the above to That sounds good to me; do we want to include "simboard" anywhere in the path to be explicit about the structure being for SimBoard? E.g., Design decision 3
Not addressed explicitly, but it seems like only |
|
@copilot Please implement these two additions to this PR:
Expected behavior:
An example cfg:
simboard_type = config["default"]["simboard_type"]
web_portal_base_path = machine_info.config.get("web_portal", "base_path")
config["default"]["www"] = (
f"{web_portal_base_path}/simboard/{simboard_type}/"should be updated so that we get something like: That is, |
Addressed in This updates the PR to use an explicit |
forsyth2
left a comment
forsyth2
left a comment
There was a problem hiding this comment.
Reviewing the diff from the last 7 commits, added by the latest call to Copilot.
✅ Add a SimBoard task.
✅ Change the inferred directory structure when using SimBoard.
| # Where the post-processing visuals should go (to be viewed online) | ||
| # Leave blank to infer `<web_portal base_path>/simboard/<simboard_type>/` | ||
| # when `infer_path_parameters = True` | ||
| # Leave blank to infer `<web_portal base_path>/diagnostics_archive/<simulation_type>/` |
There was a problem hiding this comment.
I do think we should make it something like simboard_diagnostics_archive instead.
| if not simboard_enabled(config): | ||
| raise ValueError( | ||
| "www is empty. Provide [default] www or set `enabled = True` in " | ||
| "the [simboard] section to infer a SimBoard-compatible " | ||
| "diagnostics_archive path. Note: inference requires " | ||
| "web_portal.base_path in Mache configuration." | ||
| ) |
There was a problem hiding this comment.
This block implements:
Expected behavior:
simboard.enabled |
www |
Behavior |
|---|---|---|
False |
any | zppy does nothing SimBoard-specific. |
| def infer_simboard_www(machine_info: MachineInfo, config: ConfigObj) -> str: | ||
| simulation_type = config["simboard"]["simulation_type"] | ||
| try: | ||
| web_portal_base_path = machine_info.config.get("web_portal", "base_path") |
There was a problem hiding this comment.
This line implements:
Expected behavior:
simboard.enabled |
www |
Behavior |
|---|---|---|
True |
empty | Infer the SimBoard archive path from mache. |
| except (NoSectionError, NoOptionError) as exc: | ||
| raise ValueError( | ||
| f"www is empty and simboard.enabled is True, but machine " | ||
| f"'{machine_info.machine}' has no web_portal.base_path in mache; " | ||
| "cannot infer a diagnostics_archive path." | ||
| ) from exc |
There was a problem hiding this comment.
This block implements:
Expected behavior:
simboard.enabled |
www |
Behavior |
|---|---|---|
True |
empty, but path cannot be inferred | Raise a clear configuration error. |
| web_portal_base_path = normalize_web_portal_base_path(web_portal_base_path) | ||
| if web_portal_base_path == "": | ||
| raise ValueError( | ||
| f"www is empty and simboard.enabled is True, but machine " | ||
| f"'{machine_info.machine}' has an empty web_portal.base_path in " | ||
| "mache; cannot infer a diagnostics_archive path." | ||
| ) |
There was a problem hiding this comment.
This block also implements:
Expected behavior:
simboard.enabled |
www |
Behavior |
|---|---|---|
True |
empty, but path cannot be inferred | Raise a clear configuration error. |
| inferred_www = ( | ||
| f"{web_portal_base_path}/diagnostics_archive/{simulation_type}/" | ||
| ) | ||
| logger.info( | ||
| "Inferred www=%s from mache web_portal.base_path because " | ||
| "simboard.enabled is True.", | ||
| inferred_www, | ||
| ) |
There was a problem hiding this comment.
This block finishes the implementation of
Expected behavior:
simboard.enabled |
www |
Behavior |
|---|---|---|
True |
empty | Infer the SimBoard archive path from mache. |
| if config["default"]["www"] != "": | ||
| return |
There was a problem hiding this comment.
This block implements:
Expected behavior:
simboard.enabled |
www |
Behavior |
|---|---|---|
True |
set | Use www as the output path and do not override it. simboard.enabled still controls SimBoard-specific metadata and validation. |
| inferred_www = ( | ||
| f"{web_portal_base_path}/diagnostics_archive/{simulation_type}/" |
There was a problem hiding this comment.
This block implements: f"{web_portal_base_path}/diagnostics_archive/{simboard_type}/"
| enabled = boolean(default=False) | ||
| # Use "none" only when SimBoard publishing is disabled. | ||
| # Default to "production" so enabled configs can opt in without overriding it. | ||
| simulation_type = option("production", "development", "none", default="production") |
There was a problem hiding this comment.
This line implements simboard_type = option("production", "development", "none" default="production")
There was a problem hiding this comment.
Pull request overview
This PR makes the top-level www output path configurable as “optional” by defaulting it to an empty string and introducing a SimBoard-oriented configuration hook that can infer www from Mache’s web_portal.base_path. It fits into zppy’s existing parameter inference/validation flow by extending default.ini defaults and performing early parameter determination in zppy.__main__.
Changes:
- Added a new config-only
[simboard]task hook and helpers to validate SimBoard settings and inferwwwfrom Mache (zppy/simboard.py). - Changed config defaults so
[default] wwwdefaults to"", and added a new[simboard]section withenabledandsimulation_type(zppy/defaults/default.ini). - Wired
wwwinference + SimBoard validation into startup parameter determination, and added unit tests + docs updates (zppy/__main__.py,tests/*,docs/*).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
zppy/simboard.py |
Adds SimBoard config validation and www inference from web_portal.base_path. |
zppy/defaults/default.ini |
Defaults www to empty string and introduces [simboard] defaults/validation. |
zppy/__main__.py |
Applies SimBoard validation and infers www during parameter determination; registers simboard() in launch flow. |
tests/test_zppy_main.py |
Adds unit tests for www inference, SimBoard enabled parsing, and configspec validation. |
tests/test_sections.py |
Updates expected config sections to include [simboard]. |
docs/source/parameters.rst |
Documents new SimBoard parameters and www defaulting/inference behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| inferred_www = ( | ||
| f"{web_portal_base_path}/diagnostics_archive/{simulation_type}/" | ||
| ) |
| For the top-level ``www`` parameter: | ||
|
|
||
| * If ``www`` is undefined and ``[simboard] enabled = True``, assume it is ``<web_portal base_path>/diagnostics_archive/<simulation_type>/``. | ||
| * If ``www`` is undefined and ``[simboard] enabled = False``, ``zppy`` will raise an error. | ||
| * If ``www`` is undefined and ``[simboard] enabled = True`` but Mache does not provide ``web_portal.base_path``, ``zppy`` will raise an error. |
| # Where the post-processing visuals should go (to be viewed online) | ||
| # NOTE: no default, must be provided by user | ||
| www = string | ||
| # Leave blank to infer `<web_portal base_path>/diagnostics_archive/<simulation_type>/` | ||
| # when `[simboard] enabled = True` | ||
| www = string(default="") |
|
Remaining action items:
|
|
Thanks for the updates @forsyth2.
I agree, adding the
We can keep it generic since SimBoard is a consumer of the archive rather than the owner. Similarly, we'd follow the same model where E3SM simulations output performance/timing metadata to a generic
I forgot to discuss this with the team. I think for now we can focus on |
|
Sounds good, thanks @tomvothecoder! |
forsyth2
changed the title
www from SimBoard defaultswww inference via a new [simboard] config section
|
It looks like the Copilot review got confused by the un-updated PR description. I've had Claude write an updated description for the PR and pasted it in above. I also included Tom's expected behavior table. Claude also suggested the new title: "Add SimBoard-compatible For reference: Pasted contentSummaryAdds an opt-in Expected behavior:
ChangesNew
New module
Docs (
Tests
Behavior notes
|
| def test_determine_parameters_requires_inferable_web_root() -> None: | ||
| config = _base_config() | ||
| config["simboard"]["enabled"] = True | ||
| machine_info = _fake_machine_info() | ||
| machine_info.config.remove_option("web_portal", "base_path") | ||
|
|
||
| with pytest.raises( | ||
| ValueError, | ||
| match=( | ||
| "www is empty and simboard.enabled is True, but machine 'pm-cpu' " | ||
| "has no web_portal.base_path in mache; cannot infer a " | ||
| "diagnostics_archive path." | ||
| ), | ||
| ): | ||
| _determine_parameters(machine_info, config) |
There was a problem hiding this comment.
base_path will always be available if the machine is defined in Mache. This probably isn't needed.
There was a problem hiding this comment.
It only comes up in the tests because of the fake machine info used. That's the cause of the build error FAILED tests/test_zppy_main.py::test_determine_parameters_requires_inferable_web_root - configparser.NoOptionError: No option 'base_path' in section: 'web_portal'
There was a problem hiding this comment.
Fixed in commit Fix NoOptionError crash before SimBoard www inference. The web_portal.base_path and web_portal.base_url reads in _determine_parameters() are now wrapped in try/except blocks that catch NoSectionError/NoOptionError and fall back to "". This lets the NoOptionError propagate to infer_simboard_www(), which already has the proper handler to raise the intended ValueError with the correct message.
10b7f2a to
ee45ff1
Compare
Wrap `web_portal.base_path` and `web_portal.base_url` reads in `_determine_parameters()` with try/except to catch `NoSectionError`/`NoOptionError`, defaulting to `""`. This lets the `NoOptionError` be handled properly downstream in `infer_simboard_www()`, which raises the intended `ValueError` when `www` is empty and `simboard.enabled` is True. Fixes `test_determine_parameters_requires_inferable_web_root` which was failing with `configparser.NoOptionError` before reaching the SimBoard error-handling code.
|
@copilot I've rebased this branch off the latest |
- Add docs/source/user_guide/tasks/simboard.rst describing the [simboard] section, its expected behavior table, a configuration example, and its parameters. - Add simboard to the tasks index (list and toctree). - Update parameters.rst: www is now optional (inferred when simboard.enabled=True); add a SimBoard section parameters table at the end.
Done in the latest commit. Added |
Summary
Adds an opt-in
[simboard]configuration section that letszppyinfer the[default] wwwpath automatically instead of requiring users to set it explicitly. This targets SimBoard-compatible publishing, where the diagnostics archive path can be derived from Mache'sweb_portal.base_path.Expected behavior:
simboard.enabledwwwFalseTruemache.Truewwwas the output path and do not override it.simboard.enabledstill controls SimBoard-specific metadata and validation.TrueChanges
New
[simboard]config section (zppy/defaults/default.ini)enabled(bool, defaultFalse): opts in to SimBoard-compatible publishing.simulation_type(option:production|development|none, defaultproduction): selects the archive classification used when inferringwww.[default] wwwno longer requires an explicit value (string(default="")instead of a barestring), since it can now be inferred.New module
zppy/simboard.pysimboard(...): a configuration-only task hook (no HPC job), analogous to[bundle], that validates the[simboard]section has no subsections.simboard_enabled(config): parsesenabledfrom bool or"true"/"false"strings (case-insensitive), raisingValueErroron anything else.validate_simboard_config(config): rejectssimulation_type = "none"wheneverenabled = True.normalize_web_portal_base_path(...): strips whitespace and trailing slashes.infer_simboard_www(machine_info, config): builds<web_portal base_path>/diagnostics_archive/<simulation_type>/, raising a descriptiveValueErrorif Mache has no (or an empty)web_portal.base_pathfor the current machine.zppy/__main__.py_determine_parametersnow calls a new_set_default_wwwhelper, which:validate_simboard_config(sosimulation_typeis checked even whenwwwis already explicitly set).wwwuntouched if already provided.simboard.enabled = Trueand inferswwwviainfer_simboard_www; raises a clear error ifwwwis empty and SimBoard is not enabled._launch_scriptsnow runs thesimboardconfig-hook bundle alongside other predefined bundles, beforeclimotasks.Docs (
docs/source/parameters.rst)[simboard]parameters.wwwinference rule for the top-levelwwwparameter, including the three failure/success cases (inferred when enabled, error when not enabled, error when Mache lacksweb_portal.base_path).configuraiton→configuration).Tests
tests/test_sections.py: adds expected defaults for the new[simboard]section.tests/test_zppy_main.py(new): comprehensive coverage including:wwwinference for bothproductionanddevelopmentsimulation types.simboard_enabledparsing across bool/string/invalid inputs.wwwis preserved even when SimBoard is enabled.wwwis empty and SimBoard is disabled.simulation_type = "none"while enabled.web_portal.base_pathis missing or empty.[simboard].simulation_typevalues via ConfigObj validation.Behavior notes
wwwis set explicitly, behavior is unchanged (aside from also validatingsimulation_typeisn't"none"whenenabled = True).wwwis omitted and SimBoard is not enabled,zppynow raises an error (previously this was enforced structurally by the config spec requiringwww).Select one: This pull request is...
Small Change
a single commit.