fix: Feat/load entities from stac - #19
Conversation
Mirrors the `client.stations` namespace from dclimate-client-js (PR #10), built on tabular-py. Gridded Zarr datasets still come from `load_dataset`; point-observation station datasets (GHCND and friends) read the same way -- degrees, ISO timestamps, chained selections: async with dClimateClient() as client: stations = await client.stations.load(cid) rows = await stations.select("USW00093134").time_range(a, b).rows() # Or resolve dataset and station in one call, which is the question most # callers open a station dataset to ask: st = await client.stations.nearest(cid, lat, lon, columns=["TMAX"]) Two things differ from the JS client, both because the Python side can do better rather than because parity slipped: - Reads prefer the client's own KuboCAS over a second HTTP path, so pinning, retries and configured endpoints apply to station reads too. Outside the async context there is no KuboCAS yet, so it falls back to the plain gateway. - `nearest` takes `columns=` to mean nearest station *with usable data*. Without it, near downtown LA the closest station is 0.63 km away and has never recorded TMAX, while the closest that has is 5.4 km away. Errors are translated at the boundary, so everything escaping the client is a ZarrClientError: an unknown column is InvalidSelectionError, a well-formed query matching nothing is NoDataFoundError, and unreadable bytes are the new DatasetCorruptError -- distinct because it points at the publisher rather than the caller. Transport failures deliberately pass through untranslated: a gateway timeout is retryable and says nothing about the dataset. The translation covers the whole chain, not just `load`. `select`, `where` and `rows` raise tabular's errors too, and they are where a caller is far likelier to meet one; WrappedStationDataset forwards attribute access and re-wraps returned datasets so the boundary does not stop at the first link. tabular-py is not on PyPI yet, so it is deliberately neither a dependency nor an extra: uv resolves every extra when locking, so naming an unpublished package makes `uv sync` fail even for people who never touch station data. `client.stations` raises TabularNotInstalledError with install instructions until then, and the package imports cleanly without it. Verified end to end against bafyr4if2wbttslbxpzmro427j4l4nvcrxqo4tufuffqqmqz7afj2pxyu4a, including every README example and each error-translation path.
PyPI rejects the name `tabular-py`: it normalises onto the unrelated existing `tabularpy` project. The distribution is namespaced to `dclimate-tabular-py` instead, matching @dclimate/tabular on npm. The import name is unchanged -- `tabular_py` -- since PyPI has no opinion about it and renaming the module would churn both repos for nothing. Now that it is published, station support is an ordinary dependency rather than an install-it-yourself note, matching dclimate-client-js's unconditional dependency on @dclimate/tabular. `uv sync` resolves it from the registry, so the earlier problem -- naming an unpublished package made even a plain sync fail -- is gone. `_require_tabular` and TabularNotInstalledError are kept, with the message rewritten. They no longer guard an optional extra; they exist because the import name and the distribution name differ, so a stock ModuleNotFoundError would name `tabular_py`, which `pip install` cannot find. The test now asserts the message names the installable distribution, which is the failure mode that actually matters. Verified from a clean Python 3.12 environment with no cache: the client installs, pulls dclimate-tabular-py 0.1.0 from pypi.org, imports as tabular_py, and resolves client.stations. 298 tests pass there, up from 292 -- six of the previously reported failures were local environment damage (root-owned .venv and build directories), not code.
Three defects found in review, all about a cached StationsClient outliving the transport it was built for. A KuboCAS never reopens once closed, so __aexit__ now drops the station client rather than only closing it: keeping it would make reads after the context fail on a dead transport instead of falling back to the gateway, as the property documents. Rebuilding the namespace onto the CAS used to discard the previous gateway-backed instance and its open sources. Since the replacement is the only instance __aexit__ closes, those pools leaked; the replacement now adopts them. Building happens in a property, which cannot await, so the sources move rather than being closed there. aclose() stopped at the first failing source, stranding the rest with no way to retry them -- the list is cleared up front. It now closes every source and re-raises the first failure afterwards. Each test fails on the previous commit and passes here.
Ports three changes from dclimate-client-js so the two clients stay in step.
1. `load_entities(collection=..., dataset=...)` resolves an entity dataset
through STAC, the counterpart to `load_dataset`. Separate method rather than
a layout branch inside it: the two return different types with different
query surfaces, so folding them together would widen `load_dataset`'s return
and make every gridded caller narrow first. Follows this client's flat-kwarg
convention rather than transliterating the JS `{request, options}` shape.
2. `ResolvedDatasetDetails.layout` reads `dclimate:layout`, on both the STAC
server and IPFS catalog paths. The guard is a positive match on "tabular":
entity support postdates the field, so an item without it is a gridded one
from before the convention, and treating absence as permission would admit
exactly the Zarr items the guard exists to catch.
3. `/collections` is paginated by following rel="next". The endpoint defaults
to 10 per page and the catalogue publishes 14, so an unpaged request
returned a well-formed but short list and the last four collections lost the
title and organization that endpoint is their only source of. A next link
leaving the configured origin is dropped rather than followed.
`column_key` defaults to upper-casing, matching what this catalogue publishes.
Verified against the live catalogue, matching the JS client exactly: noaa_ghcnd
resolves to the same CID and commit, ecmwf_era5 is rejected as 'zarr', and a
10-day TMAX query returns the same station (USC00305816) and values (78, 83,
72). Pre-existing failures are unchanged -- 30 before and after, all from a
local IPFS daemon and pytest-infra issues unrelated to this work.
Mirrors the JS change. `load_entities` defaulted `column_key` to upper-casing, which is a guess at each dataset's publishing profile rather than a property of the catalogue: right for GHCND, and silently wrong for NDBC's `.spec` feed, which publishes `SwH`, `SwP` and `STEEPNESS`. The justification was wrong too. `column_key` renames columns; it does not gate access to them. Without one every column is still readable under the schema's own field names, which are what the dataset stores and so are never wrong. Worth noting a divergence this exposes: tabular_py's own `default_column_key` also upper-cases, where the JS reader defaults to identity. So dropping this default changes nothing observable on the Python side -- columns still come back as TMAX -- while the JS client now returns the schema's `tmax`. The guess simply moves one layer down rather than disappearing, and fixing it belongs in tabular_py. Callers on either client can pass `column_key` explicitly to get the naming they want, verified in both directions here.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #19 +/- ##
==========================================
+ Coverage 77.59% 78.33% +0.73%
==========================================
Files 17 21 +4
Lines 2312 2515 +203
Branches 407 441 +34
==========================================
+ Hits 1794 1970 +176
- Misses 358 373 +15
- Partials 160 172 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![da-code-reviewer[bot]](https://avatars.githubusercontent.com/u/78453614?s=60&v=4){kind=small}
| # escaping the client is a `ZarrClientError`, and a list of leaf types | ||
| # silently stops keeping it the day tabular adds one. The specific class | ||
| # stays visible in the message, which is preserved verbatim. | ||
| if isinstance(cause, DclimateTabularError): |
There was a problem hiding this comment.
MEDIUM
GeoFilterError falls into this generic branch, so invalid circle, rectangle, or polygon bounds discovered during plan()/rows() are reported as DatasetCorruptError. Map GeoFilterError to InvalidSelectionError instead.
| # send a caller to the publisher over what is usually a network blip. Checked | ||
| # before the base class below, which it descends from. | ||
| if isinstance(cause, RangeSourceError): | ||
| raise cause |
There was a problem hiding this comment.
MEDIUM
A gateway failure escapes as tabular_py.RangeSourceError, bypassing except ZarrClientError and leaking the dependency’s private error hierarchy. Translate it to a client-owned transport error such as IpfsConnectionError while preserving the cause.
| near the queried point still has a nearest entity, and the only way to | ||
| tell that apart from a good match is how far away it is. | ||
| """ | ||
| dataset = await self.load(cid, gateway_url=gateway_url, column_key=column_key) |
There was a problem hiding this comment.
MEDIUM
For gateway-backed calls, load() creates and retains a new GatewayRangeSource, but nearest() discards the dataset after returning the result. Repeated calls therefore accumulate HTTP clients/connections until aclose(); close the terminal source or reuse one source per gateway.
| ) | ||
|
|
||
| metadata: DatasetMetadata = { | ||
| "collection": resolved_collection, |
There was a problem hiding this comment.
MEDIUM
This can report the wrong collection after catalog fallback. _resolve_dataset_details() may expand a unique short name such as ghcnd to noaa_ghcnd, but that canonical name is not returned, so metadata, slug, and organization still use ghcnd. Preserve the resolved collection identity when building metadata.
| # send a caller to the publisher over what is usually a network blip. Checked | ||
| # before the base class below, which it descends from. | ||
| if isinstance(cause, RangeSourceError): | ||
| raise cause |
There was a problem hiding this comment.
MEDIUM
RangeSourceError is re-raised directly, so gateway failures bypass except ZarrClientError and expose tabular-py's private exception hierarchy. Translate it to the existing IpfsConnectionError while preserving the cause.
| # escaping the client is a `ZarrClientError`, and a list of leaf types | ||
| # silently stops keeping it the day tabular adds one. The specific class | ||
| # stays visible in the message, which is preserved verbatim. | ||
| if isinstance(cause, DclimateTabularError): |
There was a problem hiding this comment.
MEDIUM
GeoFilterError reaches this generic branch, causing invalid coordinates, inverted rectangles, and malformed polygons to be reported as DatasetCorruptError. Classify GeoFilterError as InvalidSelectionError instead.
| near the queried point still has a nearest entity, and the only way to | ||
| tell that apart from a good match is how far away it is. | ||
| """ | ||
| dataset = await self.load(cid, gateway_url=gateway_url, column_key=column_key) |
There was a problem hiding this comment.
MEDIUM
Without a CAS—or with gateway_url overridden—load() creates and retains a new GatewayRangeSource, but nearest() discards the dataset after returning. Repeated terminal calls accumulate HTTP clients until aclose(); reuse the source or release it after this operation.
| ) | ||
|
|
||
| metadata: DatasetMetadata = { | ||
| "collection": resolved_collection, |
There was a problem hiding this comment.
MEDIUM
This metadata can contain the wrong collection after catalog fallback. _resolve_dataset_details() may expand a unique short name such as ghcnd to noaa_ghcnd, but that canonical name is not returned, leaving collection, slug, and organization based on the original short name. Preserve the resolved collection identity.
| # send a caller to the publisher over what is usually a network blip. Checked | ||
| # before the base class below, which it descends from. | ||
| if isinstance(cause, RangeSourceError): | ||
| raise cause |
There was a problem hiding this comment.
MEDIUM
Gateway and range failures escape as tabular_py.RangeSourceError, which is not a ZarrClientError. This breaks the documented single-exception boundary and leaks the dependency’s error API; translate it to IpfsConnectionError while preserving the cause.
| cas = self._kubo_cas | ||
| current = self._entities_client | ||
| if current is None or current._cas is not cas: | ||
| replacement = EntitiesClient( |
There was a problem hiding this comment.
MEDIUM
The supported outside-context fallback constructs EntitiesClient without the parent’s headers, auth, client_factory, or retry settings. Consequently, a protected gateway configured on dClimateClient works through KuboCAS inside the context but fails (typically with 401) through client.entities outside it. Propagate transport configuration to the default-gateway source.
| results use. A dataset's published column names are a property of its | ||
| profile, not of the stored blocks: GHCND stores a field named ``tmax`` | ||
| and publishes it as ``TMAX``, NDBC preserves mixed case like ``SwH``. | ||
| The reader's default is the identity -- the schema's own field names -- |
There was a problem hiding this comment.
LOW
This documents the reader default as identity, but pinned tabular_py 0.2.1 defaults to field.name.upper(). Callers following this documentation and querying stored names such as tmax receive InvalidSelectionError; document the uppercase default or explicitly pass an identity mapping.
2 participants
No description provided.