feat(esm_catalog): PR-A1a — STAC data-model foundation#1484
Conversation
43ba2c3 to
8d58172
Compare
541592a to
7e5af57
Compare
|
Rebased onto the corrected PR-0 base (#1483) — test relocation to |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ependency) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… unused field, tighten guard test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
7e5af57 to
d995632
Compare
There was a problem hiding this comment.
Hi @siligam,
I just reviewed hpc/detect.py. I am suggesting some design changes to reuse some of our detect infrastructure and also to make the functions location-agnostic.
I think I'll do more of these "incomplete" reviews, so that you can start working on certain things.
| "hpc:facility": "AWI", | ||
| "hpc:system": "albedo", | ||
| "hpc:storage_type": "lustre", | ||
| "hpc:state": "online", |
There was a problem hiding this comment.
This belongs to the configs/machine/albedo.yaml
| "hpc:facility": "DKRZ", | ||
| "hpc:system": "levante", | ||
| "hpc:storage_type": "lustre", | ||
| "hpc:state": "online", |
There was a problem hiding this comment.
This belongs to the configs/machine/levante.yaml
| path_str = str(Path(path).resolve()) | ||
|
|
||
| # AWI Albedo cluster (Lustre) | ||
| if "/albedo/" in path_str: |
There was a problem hiding this comment.
You can detect the machine and which yaml file to load with:
| # Tape/HPSS archive paths | ||
| if "/arch/" in path_str or "/hpss/" in path_str: | ||
| return { | ||
| "hpc:storage_type": "hpss", | ||
| "hpc:state": "offline", | ||
| "hpc:recall_time_estimate": 300, | ||
| } |
There was a problem hiding this comment.
We need an strategy for tape archives that are not registered in the config/machine yamls. One could simply have a storage dir in the configs with 1 yaml file per storage with the following info:
configs/storage/hpss.yaml
storge:
storage_type: "hpss"
state: offline
recall_time_estimate: 300
path_str: "/arch/|/hpss/"There was a problem hiding this comment.
I agree. How about attaching it to the computer and having some useful objects for Storage with attributes and methods like get/put?
computer:
name: albedo
storage:
- cls: disk
name: albedo-work
type: pfs
... : lustre # I don't know lhs here. Maybe 'provider' (others would be gpfs, gifs, bgfs, and one base class generic-parallel-filesystem)
state: online
path_prefix: "/albedo/work"
comment: "... something we can use in functions consuming a SimulationStorage object)Then we have objects in Python like:
@dataclass
class DiskSimulationStorage:
name: str
...
class ScoutFSTapeSimulationStorage:
def get(p: UPath) -> UPath: ...This idea could be expanded...
There was a problem hiding this comment.
Sorry, my example above is missing the specific part that makes this interesting: computer gets an attribute called storage, which is a list of things. One of those things is an object representing the tape system that this particular HPC can communicate with.
There was a problem hiding this comment.
I agree. How about attaching it to the computer and having some useful objects for
Storagewith attributes and methods like get/put?computer: name: albedo storage: - cls: disk name: albedo-work type: pfs ... : lustre # I don't know lhs here. Maybe 'provider' (others would be gpfs, gifs, bgfs, and one base class generic-parallel-filesystem) state: online path_prefix: "/albedo/work" comment: "... something we can use in functions consuming a SimulationStorage object)Then we have objects in Python like:
@dataclass class DiskSimulationStorage: name: str ... class ScoutFSTapeSimulationStorage: def get(p: UPath) -> UPath: ...This idea could be expanded...
I find this idea a bit over engineer for what we currently need. Additional features can always be added in the future but we need a minimal product before the end of the year and we are very delayed.
I will tweak @pgierz's approach to the following:
machines/albedo.yaml
computer:
name: albedo
[ ... ]
storage:
albedo-gpfs: # Not a list but a dictionary
type: gpfs
state: online
as_few_parameters_as_possible: we can add more parameters later on when needed for other functionalities from the requirementsmachines/awi-hsm.yaml
storage:
awi-hsm:
type: tape
state: offlineConsider which parameters need to be set dynamically (to be extracted via functions in the catalog module), and which ones can be static (defined in yaml files). My guess is that almost everything can static.
There was a problem hiding this comment.
Sorry, my example above is missing the specific part that makes this interesting: computer gets an attribute called storage, which is a list of things.
I don't like this. Computer should no have storage inside, as storage can be an independent entity from the HPC (e.g. tape).
One of those things is an object representing the tape system that this particular HPC can communicate with.
Yes, but you can also communicate with a tape storage system from somewhere else right? I wouldn't make it an object for now anyway.
There was a problem hiding this comment.
Can you make this function machine agnostic? Specific details can be stored in config/machine/ yamls. See my comments below :)
|
Hi Miguel, thanks for the review — agreed on the core idea: the facility/storage metadata shouldn't be hardcoded path-string checks in Python, and your One thing I want to flag before implementing: hooking into What I'd like to do instead: keep the matching path-pattern-based (as it is today), but move the per-machine/per-storage metadata out of Python and into small yaml files that Does that work for you, or did you have a reason for wanting this to live in the shared |
|
Hi @siligam, I have a feeling this is going straight to @claude with some additions of your own, so here are instructions you can add to your own prompts:
Mixing together I am not sure where @claude got the idea that this needs to be entirely separate and modular, but, it doesn't. @mandresm will remember the nightmare it was trying to keep separate packages for
OK, but...
...very strong NO!!. We have
See the comment above. This needs serious human discussion first, and if we go in the direction I imagined, we might need a whiteboard session with the rest of the core developers (@mandresm especially, but I would be interested in seeing @JanStreffing and @seb-wahl's input if they have time)
It should be in the global data tree because (and I would hope that this should be obvious) we do not want to maintain more than one source of truth. The chance of drift (especially in the age of AI coding) is far too large if we do not have a strict N=1 policy for where program state comes from. |
|
@pgierz Got it, thanks. I'll hold off on the detect_hpc_storage() redesign until after the core-dev discussion, but will go ahead and drop the esm_parser-coupling concern / stop reinventing yaml loading in the meantime. |
Co-authored-by: Claude <noreply@anthropic.com>
- Drop stac/ and extensions/ subdirectories; all files go flat in esm_catalog/ - Custom extension schemas (hpc, paleo, namelist) go in configs/stac-extensions/ - registry.py keeps esm-tools.github.io placeholder URLs; each extension PR updates to raw GitHub URL after schema is merged to release - Consolidate hpc/detect.py + stac/extensions/hpc.py into single hpc.py Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- ESMCollection/ESMItem as dict subclasses - flat layout (no stac/ or extensions/ subdirs) - test_no_scan_dependency.py deferred to scan layer PR Co-Authored-By: PavanSiligam <pavan.siligam@gmail.com> Co-Authored-By: Paul Gierz <pgierz@awi.de> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Hi @siligam, I am breaking down this PR in even more pieces. I have created this plan with claude-code: https://github.com/esm-tools/esm_tools/blob/22519379dc96e17e65d8d302c6d969375f0ec139/src/esm_catalog/PR-A1A-STAC-FOUNDATION-PLAN.md And I have already start implementing that plan, see #1504. This PR would be close without merging once all the steps are completed. I think this is best as we remove the back and forth of me asking for changes to follow the ESM-Tools software conventions and you having to implement them. The good news is that you can also review my PRs so that you get an idea of what my suggestions would be anyway. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tation - function-based API (make_item/make_collection) over dict subclasses - pystac.Item / pystac.Collection as return types - CollectionContext production validation and data_license field - no esm_tools dependency from esm_catalog Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@siligam, replying to some of your comments here:
Those are all very good points and the solution you are proposing makes sense to me.
Yeah, all configs should live in the same
Dependency-free is not a priority for me. But I don't think we need the esm_parser so we can avoid using the esm_parser here for sure. I can guess we are going to need esm_parser to generate the config from a given runscript in the future, but for now, we can live in an "easier" world where we can specify which yaml we read. |
|
This is about what @pgierz said:
Similar to what I said. Keep things separated if easy, possible and no duplication.
I don't see why this is so crucial to discuss now. I would choose for now static definitions, create a new root key |
- Contact dataclass in context.py; contacts via CollectionContext - general.contacts list replaces ai-invented pi_name/PI_name keys - production validation requires at least one contact with institution Co-Authored-By: PavanSiligam <pavan.siligam@gmail.com> Co-Authored-By: Paul Gierz <pgierz@awi.de> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks Miguel — that all makes sense, and dict-not-list for
storage:
albedo-work:
storage_type: lustre
state: online
storage:
hpss:
storage_type: hpss
state: offline
recall_time_estimate: 300
path_str: "/arch/ |/hpss/"
For loading, I'll use Will start on this unless you see a problem with reading configs that way. |
…l configs/ Per PR-A1a review discussion (#1484): machine/facility storage metadata now lives in configs/machines/*.yaml and configs/storage/*.yaml (the shared esm-tools config tree) under a new `storage:` root key, instead of being hardcoded path checks in Python. detect_hpc_storage() loads these via esm_parser.yaml_file_to_dict and matches path_str regexes against the given path, falling back to statvfs probing when nothing matches. This makes esm_catalog depend on esm_parser for the first time. Its CI (esm-catalog-tests.yml) drops the --no-deps/3.12 workaround and matches the rest of esm-tools at Python 3.9, since esm_parser's ruamel.yaml.clib==0.2.7 pin doesn't build on 3.12.
|
Pushed (61b1fbf): One consequence worth flagging: this is the first time Added |
The broad "/work/ and not /levante" pattern matched any /work/ path regardless of machine, and could shadow another machine's own /work/-based pattern once one is added (alphabetical load order puts several after levante.yaml). Anchor to the actual DKRZ project-code convention instead (/work/<2 letters><4 digits>/, verified against real paths already present in this repo's configs/runscripts) — this also makes the negative lookahead for "/levante" redundant, since that string can never fill a digit slot. Adds regression coverage for the shadowing case, plus two coverage gaps (remote-path fallback, real unmocked statvfs fallback) surfaced during external review of 61b1fbf.
|
Had a second look at 61b1fbf and evaluated a few follow-up findings:
Note 1 and 2 weren't regressions introduced by 61b1fbf — both existing heuristics were carried over faithfully from the original hardcoded Python. Worth tightening regardless, done for Levante. |
|
This PR is now fully superseded and can be closed: everything the plan carved out of it exists as its own PR — A1b (#1504) and the test coverage (#1508) are merged, and A1c (#1511), A1d (#1507), A1e (#1512), A1f (#1513), A1g (#1506) are open. The plan file on this branch is ticked through A1g. @mandresm you said you'd close this once the steps were done — I think we're there, so feel free to close from my side whenever you are. One loose end for the record: the recent commits here (61b1fbf, 71ca077) added |
PR-A1a — STAC data-model foundation (+ scan⇄stac cycle-break)
Second slice of #1473's decomposition. Lands the STAC data model so it stands alone — reviewable and testable without the scan, storage, or api layers present.
What's here (+1,502 lines, 21 files)
stac/—collection.py,item.py, and extensions (datacube,contacts,hpc,paleo,namelist,registry)hpc/— dependency leaf (storage-state detection)uri.py(new, neutral) —parse_uri/to_uri, moved out ofscan/upath.pycontext.py(new, neutral) —CollectionContext, carrying pre-scannednamelists_by_componentThe cycle-break (the point of this PR)
stac/previously reached intoscan/through three lazy imports — a hidden dependency cycle that hurts reviewability/maintainability. This PR removes all of it:esm_catalog.uriCollectionContext→ neutralesm_catalog.contextmake_collectionandadd_namelist_item_extensionare now pure — they receive pre-scanned namelist data instead of scanning. The scanning itself moves to the scan layer (next PR, A1b).tests/test_no_scan_dependency.pykeeps it broken: an AST test fails if anystac/file importsesm_catalog.scan, plus a runtime test that reloads the stac modules with scan imports blocked.Tests
15/15 pass (
pytest src/esm_catalog/tests), Python 3.12.Context
Full plan: decomposition design. Follows: PR-A1b (scanners), which reintroduces the namelist scanning and
scan→stac(one-directional) usage.