docs: OED load/validate notebook + settings-schema reference - #289
Open
sstruzik wants to merge 10 commits into
Open
docs: OED load/validate notebook + settings-schema reference#289sstruzik wants to merge 10 commits into
sstruzik wants to merge 10 commits into
Conversation
Stand up an executable docs project in ODS_Tools (Furo + myst-nb) and add tutorials/load-validate-oed.md: loads an OED location file with ods_tools.oed (typed DataFrame), runs the OED validation rules in 'return' mode (catches a missing conditionally-required LocPeril), fixes it and re-validates (0 findings). This exercises the ods_tools LIBRARY, so it executes at docs-build (verified). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a reference section generated at build time from the bundled JSON Schemas (model/analysis/combine settings) via a recursive schema walker (_ext/gen_settings_reference.py), expanding nested objects (e.g. gul_summaries.ord_output) into their own tables. Wire cross-component intersphinx links (e.g. analysis_settings -> ORD tables). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add the Oasis logo (light/dark, always visible in the Furo sidebar), the maroon/red Raleway palette matching oasislmf.github.io, and a persistent 'Oasis documentation home' link (rewritten to page-relative by the aggregator) so you can return to the top of the aggregated site from anywhere. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The sidebar logo now returns to the aggregated landing, so the redundant 'Oasis documentation home' announcement bar is removed. Add the GitHub link in Furo's conventional spot — the footer icons (bottom of every page). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #289 +/- ##
=======================================
Coverage ? 74.10%
=======================================
Files ? 214
Lines ? 14140
Branches ? 0
=======================================
Hits ? 10478
Misses ? 3662
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sstruzik
marked this pull request as ready for review
August 11, 2026 08:58
Split a combined 'import json, os' onto separate lines so the Code Quality autopep8 check passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Ha-Ree
requested changes
Aug 14, 2026
…data
Four fixes, one per review thread group.
**gen_settings_reference: report the constraints the schemas actually carry.**
_constraints() only understood enum/default/minimum/maximum, so 95 keyword
occurrences across the three schemas were dropped (minLength x69, uniqueItems x16,
minItems x6, minProperties, maxLength, pattern, multipleOf). Worse, 29 array
properties keep their constraints on items{}, which was never inspected — including
7 items.enum allowed-value lists, so group_event_set_fields, group_format_priority,
valid_output_perspectives/metrics and the event_set valid_* fields all rendered an
EMPTY Constraints cell. A generated reference silently omitting which values are
legal defeats the point of generating it. Arrays are now summarised in two parts:
keywords on the array describe the list, item keywords are reported as "each item
...". Arrays of objects are left alone since _object_children already expands those
into their own subsection. Result: 89 -> 212 of 335 properties document a
constraint; group_event_set_fields now lists all 9 values.
**conf.py: drop the guards that this file makes unreachable.** The shared
cross-component block was written to be paste-safe into any conf.py, but here every
branch is decidable: os was already imported at line 6, sphinx.ext.intersphinx is
provably absent from the extensions literal, intersphinx_mapping/html_theme_options/
html_static_path/html_css_files are provably undefined, and html_theme is set to
furo 20 lines above. Replaced with straight assignment, and json moved up to the
other imports. (The block was no longer uniform across the six repos anyway — the
E401 fix in eb349ea had already forked it.)
**conf.py: no more 'ord' inventory warning on a standalone build.** The one
{external+ord:doc} reference in reference/analysis-settings.md can only resolve when
the GenerateDocs orchestrator supplies OASIS_INTERSPHINX_MAP, so building this repo
alone always warned about something the build cannot know. intersphinx.external is
now suppressed only when no map is supplied; a mapped build stays strict. Verified
all three ways: standalone -> 0 warnings; ord mapped -> resolves, 0 warnings; mapped
WITHOUT ord -> still warns.
**tutorials: stop duplicating the OED sample.** The location file under
tutorials/data/oed/ was a byte-identical copy of validation/
SourceLocOEDPiWind10Currency.csv. The notebook now walks up from the build cwd to
find the repo's own copy, so there is one sample, not two.
Verified: sphinx -E is 0 warnings standalone (was 1), the notebook still executes at
build with identical output (10 locations, 25 columns, 1 finding -> 0 after fix),
branding and both logos intact, autopep8 and the CI flake8 selection clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to 50f16e8, which fixed the 'ord' inventory warning the wrong way. Suppressing intersphinx.external on standalone builds silenced the warning but left the underlying damage in place: an unresolved {external+...} role does NOT degrade to plain text — Sphinx drops the link text entirely, so the sentence rendered as "...each maps to a table in the (e.g. ept_full_uncertainty_oep ...)". The warning was the only signal that the prose was broken, and suppressing it hid a visible defect instead of fixing one. Written as an ordinary link to the published address, the sentence is correct in every build mode, and nothing needs suppressing — so the conditional goes too rather than sit there as dead code. Relocatability is preserved by the orchestrator, not by intersphinx: rewrite_cross_links() matches any href under site_base_url whose path starts with a known module, so this link becomes ../../ord/reference/tables.html in the assembled site exactly as an intersphinx-emitted one would (verified by running that function over the built output: -> ../../ord/reference/tables.html). What this trades away is build-time verification that the target page exists; that belongs to a linkcheck job over the assembled site, where it can check every component's links rather than just the ones expressed as roles. The conf.py comment now records the trap so the next author doesn't reintroduce it. Verified: standalone sphinx -E is 0 warnings with the link and full sentence rendered; autopep8 and the CI flake8 selection clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… review) cc5416e swapped the {external+ord:doc} role for a plain link so the sentence would survive a standalone build. That fixed the prose but threw away what the role was for: Sphinx no longer verifies that ORD's page still exists, so a rename there rots this link silently. ODS_OpenResultsData#11 is the PR standing that site up for the first time, so that is the link most likely to move and least likely to be watched. Both properties are available, because the two failure modes are not the same: missing INVENTORY (standalone) -> intersphinx.external, link text DROPPED missing TARGET (orchestrated) -> ref.doc warning, degrades to plain text Only the first mangles the prose. So the role is used exactly where it can be checked. conf.py picks the form from the same env var that drives intersphinx_mapping, and the page carries a substitution rather than a hand-written target: orchestrated -> {external+ord:doc}`ORD standard <reference/tables>` standalone -> [ORD standard](https://oasislmf.github.io/ord/reference/tables.html) Verified by simulating the rename this is meant to catch — pointing the role at reference/result-tables with the inventory present gives: analysis-settings.md:9: WARNING: external std:doc reference target not found: reference/result-tables [ref.doc] and the sentence still reads correctly, with the title rendered as plain text. This also removes a drift risk the plain link carried. GenerateDocs rewrites in-site links by matching site_base_url + a known module path, so had `ord` been renamed in modules.json the hand-written URL would have stopped matching and been published as-is, 404ing quietly. Under the role the base comes from the orchestrator's own map, so it follows modules.json automatically. Separately hardens the env parsing: `environ.get(NAME, "{}")` only defaults when the variable is UNSET, so exporting it empty reached json.loads("") and aborted the build with a traceback. Now `environ.get(NAME) or "{}"`. The same latent flaw is in the other five components' conf.py; worth fixing there if you agree it is worth a follow-up. Verified: 0 warnings with the variable unset, set-but-empty, and mapped; the rendered link checked in each; autopep8 and the CI flake8 selection clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The paragraph still told the next author to write an ordinary link instead of a role, which was true of cc5416e but not of 10e38ba — the role is now used wherever it can actually be checked. Leaving it would have been the same defect we just fixed in Specification.md: guidance contradicting the code beneath it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ha-Ree
reviewed
Aug 21, 2026
|
|
||
|
|
||
| def _cell(text): | ||
| return str(text).replace("|", "\\|").replace("\n", " ").replace("\r", " ").strip() |
Contributor
There was a problem hiding this comment.
Can be expanded to include < and > so events_<id>.bin etc load (currently says events_.bin
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
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.
Stands up documentation for ODS_Tools: an executable OED load-and-validate tutorial, and a settings-schema reference generated from the schemas this repository owns.
Part of the cross-repo Oasis documentation restructure (Option B): each repository owns the docs for what it owns, and
GenerateDocsaggregates them into the published site.What this adds
tutorials/load-validate-oed— a myst-nb notebook that executes at build time: loads an OED location file withOedExposure, runsDEFAULT_VALIDATION_CONFIGin return mode, catches a missingLocPeril, fixes it and re-validates. Because it runs, its output cannot drift from the library.ods_tools/data/*_settings_schema.json(_ext/gen_settings_reference.py), covering model, analysis and combine settings. The schemas stay the single source of truth — edit the JSON, not Markdown.Review-round changes (@Ha-Ree)
The generated reference was dropping most of the constraints in the schemas.
_constraints()understood onlyenum/default/minimum/maximum, so 95 keyword occurrences were silently omitted (minLength×69,uniqueItems×16,minItems×6,minProperties,maxLength,pattern,multipleOf). Worse, 29 array properties keep their constraints onitems{}, which was never inspected — including 7items.enumallowed-value lists, sogroup_event_set_fields,group_format_priority,valid_output_perspectives/metricsand theevent_setvalid_*fields all rendered an empty Constraints cell. A generated reference that silently omits which values are legal defeats the point of generating it. Arrays are now summarised in two parts — keywords on the array describe the list, item keywords are reported as "each item …" — and arrays of objects are left alone, since they already expand into their own subsection. Coverage: 89 → 212 of 335 properties now document a constraint.The
ordcross-reference took two attempts, and the first was wrong. The page's one{external+ord:doc}reference warned on every standalone build, because the ORD inventory only exists when the orchestrator suppliesOASIS_INTERSPHINX_MAP. I first suppressed the warning — which hid a visible defect: an unresolved{external+...}role does not degrade to plain text, Sphinx drops the link text, so the sentence rendered as "…a table in the (e.g. …)". It is now routed through a per-build-mode substitution inconf.py: the role under the orchestrator (verified, and it warns if ORD renames the page — confirmed by simulating exactly that), a plain link standalone. Verified all three ways: standalone 0 warnings;ordmapped resolves; mapped withoutordstill warns.conf.pytidied — the guards this file makes unreachable are gone (oswas already imported,sphinx.ext.intersphinxis provably absent from the extensions literal,intersphinx_mapping/html_theme_options/html_static_path/html_css_filesare provably undefined,html_themeis set 20 lines above). One correction to the premise:jsonwas not imported at the top, so it moved up rather than being deleted. Also hardened againstOASIS_INTERSPHINX_MAPbeing exported empty, which reachedjson.loads("")and aborted the build.Duplicate sample data removed — the tutorial's OED file was byte-identical to
validation/SourceLocOEDPiWind10Currency.csv; the notebook now walks up from the build directory to the repository's own copy, and still executes with identical output (10 locations, 25 columns, 1 finding → 0 after the fix).Build is 0 warnings standalone and in the aggregated build.
One open question for the reviewer
Dropping the role in favour of a substitution means Sphinx no longer verifies the ORD target on a standalone build (the orchestrated build still does). GenerateDocs#55 adds a weekly link check over the assembled site, which covers this and every other link — worth confirming that is where you would want that check to live.
Add ODS_Tools documentation: an executable OED load/validate tutorial and a settings-schema reference generated from the bundled schemas.