Follow-ups: variant provenance, mypy 0 + py.typed, KuboCAS knobs, httpx consolidation (+ upstream review-commit merge) - #15
Conversation
…data and page-local preference Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ne crash, and kwargs validation Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… the variant actually selected resolve_cid_from_stac_server and resolve_dataset_cid_from_stac now return a ResolvedDataset NamedTuple instead of a bare CID string (breaking change, noted in docstrings), so load_dataset metadata and slugs reflect the preference cascade's actual pick instead of claiming 'default'; the direct-CID path reports 'unknown' consistently in both slug and metadata. The no-variant pagination path now stops early only on an effective-default match, so a 'latest' item on page 1 no longer beats a 'default' item on page 2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… crash; validate geo kwargs - implicit-Optional defaults corrected across client.py and geotemporal_data.py; load_dataset narrowing restructured so the checker proves collection/final_cid are str; siren typing fixes - dClimateClient with gateway_base_url=None no longer crashes the STAC-catalog fallback (AttributeError on None.rstrip); the default public gateway constant is used - GeotemporalData.query validates required point/circle/rectangle kwargs and raises InvalidSelectionError naming missing keys instead of failing deep inside xarray - mypy pinned in the testing extra with [tool.mypy] config; a meta-test gates it; py.typed shipped and verified present in the built wheel Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ipt presence Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…empty-variant edge, ResolvedDataset polish) Per grok + adversarial review: version bumped to 0.6.0 so the 'Changed in 0.6' notes on the breaking ResolvedDataset return are true; mutation testing showed the default>final cascade order and the first-match fallback were unpinned — both now have tests (fallback must report the item's actual variant, never a fabricated 'default'); variant='' is documented and pinned as an explicit variant; pagination tests pin resolved.variant; isinstance replaces hasattr; NamedTuple import style. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…way h2 benchmark script dClimateClient now accepts concurrency, headers, auth, max_retries, initial_delay, backoff_factor and client_factory, forwarded to KuboCAS only when set so py-hamt's defaults stay authoritative — unblocking authenticated gateways, tuned pools, and h2-on/off comparisons. scripts/benchmark_gateway.py implements the py-hamt #58 methodology (median wall time, --http2/--no-http2, --concurrency, --repetitions) and documents the companion infra change: raising nginx keepalive_requests on the gateway to stop routine GOAWAY churn. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ncy list, pooled client) + fallback non-regression pin Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…llib3 stac_server and stac_catalog use pooled sync httpx.Clients (IPFSStacIO keeps a sync client because pystac's StacIO interface is synchronous); fallback guards in dclimate_client catch httpx.HTTPError; the connection-error classifier drops the requests/urllib3 families; the nine transport-pinned test files migrate to an httpx.MockTransport seam with every invariant preserved (request bodies, pagination sequences, single-pooled-client reuse, the event-loop heartbeat bound); requests and urllib3 leave [project] dependencies (still present transitively). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ttpx/ResolvedDataset line Reconciliation decisions: - upstream's requests-era additions translated to httpx: headers-aware _search_pages (pooled module client), get_root_catalog_cid(catalog_url), conftest RPC/pointer probes - upstream's known_datasets longest-match parsing and collect-all-pages resolve adopted wholesale, fused with ResolvedDataset returns and _effective_variant selection; the 'default on page 1 stops paging' optimization is superseded by upstream's authoritative full walk (call-count pin updated 1 -> 2 accordingly) - upstream's per-catalog stac_io binding + weakref close lifecycle adopted; IPFSStacIO now owns a per-instance httpx.Client (closable) while one-shot pointer reads keep the module-pooled client - upstream's urllib3 MaxRetryError refinements dropped as moot on the httpx stack (HTTPStatusError already classifies non-connection) - upstream's new tests ported from requests monkeypatching to the httpx MockTransport seams with all assertions preserved Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- IPFSStacIO.read_text implements the http(s) fallback for real (the old super().read_text was abstract and raised NotImplementedError behind a type-ignore); unsupported schemes raise a clear ValueError - query() kwargs validation is value-aware (required keys must be non-None) and consistent: empty polygon/multiple_points kwargs now raise like the other selectors - dClimateClient: eager ValueError for client_factory combined with headers/auth (KuboCAS rejected it only at context entry); gateway-None split semantics documented and the fallback ternary hoisted; auth forwarding pinned in tests - mypy meta-test runs the project-pinned mypy and asserts the checked file count; py.typed presence pinned inside a built wheel - benchmark: one-line failures to stderr, follow_redirects=True factory, JSON-only stdout - probe redirect parity, managed mock clients in test helpers, stale requests.HTTPError reference in DATASET_CATALOG_USAGE.md Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![da-code-reviewer[bot]](https://avatars.githubusercontent.com/u/78453614?s=60&v=4){kind=small}
| max_retries: typing.Optional[int] = None, | ||
| initial_delay: typing.Optional[float] = None, | ||
| backoff_factor: typing.Optional[float] = None, | ||
| client_factory: typing.Optional[typing.Callable[[], httpx.AsyncClient]] = None, |
There was a problem hiding this comment.
MEDIUM
client_factory exists only in the unreleased py-hamt commit selected by [tool.uv.sources]; the published py-hamt==3.4.1 constructor does not accept it, while the runtime dependency still permits that release. A normal wheel install using this option therefore raises TypeError in __aenter__. Require a published py-hamt release containing this API or defer exposing the option.
| self._stac_catalog = await asyncio.to_thread( | ||
| load_stac_catalog, | ||
| gateway_url=self._gateway_base_url, | ||
| gateway_url=self._catalog_gateway_base_url, |
There was a problem hiding this comment.
MEDIUM
The catalog fallback forwards only the gateway URL. headers and auth go exclusively to KuboCAS, while IPFSStacIO creates a credential-free client, so authenticated gateways fail with 401 whenever STAC-server lookup is unavailable or disabled (including catalog-backed listing). Thread gateway credentials into catalog I/O and apply them to gateway requests.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14323331dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "max_retries": self._max_retries, | ||
| "initial_delay": self._initial_delay, | ||
| "backoff_factor": self._backoff_factor, | ||
| "client_factory": self._client_factory, |
There was a problem hiding this comment.
When this package is installed from a built wheel/PyPI, [tool.uv.sources] is ignored and the declared runtime dependency remains py_hamt>=3.4.1; the published v3.4.1 KuboCAS.__init__ accepts client=, headers/auth/retry knobs, etc., but not client_factory. Any user following the new client_factory API (and the new benchmark script does this) will therefore hit TypeError: KuboCAS.__init__() got an unexpected keyword argument 'client_factory' on async with dClimateClient(...). Please either raise the runtime floor to a released py-hamt version that includes this parameter or avoid forwarding this kwarg until that release exists.
Useful? React with 👍 / 👎.
| """ | ||
| if root_cid is None: | ||
| root_cid = get_root_catalog_cid(catalog_url) | ||
| root_cid = get_root_catalog_cid(catalog_url, headers=headers, auth=auth) |
There was a problem hiding this comment.
HIGH
Gateway headers/auth are forwarded to catalog_url, but dClimateClient leaves that URL at the fixed public STAC_CATALOG_URL. Configuring a custom authenticated gateway therefore sends its credentials to a different host. Only forward credentials when origins match, or expose separate pointer credentials.
|
|
||
| scheme = urlsplit(source_text).scheme.lower() | ||
| if scheme in {"http", "https"}: | ||
| response = self.client.get(source_text) |
There was a problem hiding this comment.
HIGH
This client was constructed with private gateway headers/basic auth, but it is reused for arbitrary absolute HTTP(S) links supplied by the catalog. Cross-origin links therefore receive gateway credentials, potentially even over HTTP. Use an uncredentialed client for external links or restrict credentialed requests to the gateway origin.
| "numcodecs[crc32c]>=0.14,<0.16", | ||
| "numpy>=2.1.3", | ||
| "py_hamt>=3.4.1", | ||
| "py_hamt>=3.5.0", |
There was a problem hiding this comment.
MEDIUM
uv.lock still resolves py-hamt 3.4.1 from git revision 942580d… and retains that source in root metadata, while this line requires registry version >=3.5.0 and removes the source override. Locked syncs will fail freshness checks, while frozen syncs install the old integration build. Regenerate and commit the lockfile.
2 participants
Stacked on #14 (base:
fix/review-findings). This PR implements the five follow-ups documented at the end of #14 through the same TDD pipeline — red/green commit pairs, each package dual-reviewed (Grok + adversarial Claude) with reviewer findings landed as remediation commits — and merges the maintainer review commits pushed to #14 (db7444b..179dd78) with full semantic reconciliation onto this line.Results: 232 tests pass (up from 190 pre-merge, 166 at the base of #14), mypy 0 errors and
py.typedshipped (verified inside the built wheel and end-to-end from a consumer venv), requests/urllib3 fully removed, version bumped to 0.6.0 for the one deliberate breaking change. Final adversarial gate executed every attack live: zero blockers, zero minors.1. Variant provenance + pagination preference (FU-A)
load_datasetmetadata reported the caller'svariantargument, not what the resolver'sdefault > final > finalized > latestcascade actually picked — a silent provenance error whose slug could name a nonexistent item. Breaking change: both resolvers now returnResolvedDataset(cid, variant); metadata/slug reflect the real selection; direct-CID paths report"unknown"consistently."default");variant=""documented+pinned as explicit.2. mypy 40 → 0,
py.typedshipped (FU-B)load_dataset, siren typing); latent crash:gateway_base_url=NonehitNone.rstripin the catalog fallback (now uses the default-gateway constant);query()kwargs with missing keys died deep in numpy (nowInvalidSelectionErrornaming the key).py.typedshipped; PEP 561 promise verified by installing the wheel in a throwaway venv and type-checking a consumer script.IPFSStacIO.read_text's "HTTP fallback" called an abstract super and always raised; it now actually fetches http(s) URLs and rejects unknown schemes. Value-aware kwargs validation (lat=Noneno longer reaches numpy); mypy meta-test pins the checked-file count so a futureexcludecan't game it.3. KuboCAS knobs + h2 benchmark (FU-C)
dClimateClientexposed none of KuboCAS's connection knobs — authenticated gateways, tuned concurrency, and h2-on/off comparisons required bypassing the client entirely. Now forwardsconcurrency,headers,auth,max_retries,initial_delay,backoff_factor,client_factory(only when set,is not None-gated — explicitmax_retries=0survives; the adversarial reviewer replayed every recorded kwargs set against the realKuboCAS.__init__).scripts/benchmark_gateway.pyimplements the py-hamt #58 methodology and documents the companion nginxkeepalive_requestsinfra change.ValueErrorforclient_factory+headers/auth(KuboCAS rejected it only at context entry); benchmark: one-line failures,follow_redirects=True, JSON-only stdout.4. requests → httpx consolidation (FU-D)
StacIOis a synchronous interface), fallback guards catchhttpx.HTTPError, the connection-error classifier drops the requests/urllib3 families, and nine transport-pinned test files moved tohttpx.MockTransportseams with every invariant preserved (assertion counts audited pre/post — none lost).requestsandurllib3left[project] dependencies.5. Upstream merge (a3fb3ca)
The maintainer commits on #14 were written against the requests-era code; this merge preserves both lines:
known_datasetslongest-match parsing for hyphenated ids, collect-all-pages resolve, per-catalogstac_iobinding +weakref.finalizeclient lifecycle (replaces the process-globalStacIO.set_defaultrace),STAC_CATALOG_URLpointer/data-gateway separation, next-link headers (including the header-aware pagination loop guard),__aexit__body-cancellation precedence,polygons()CRS-units fix,points()empty/non-finite guards,to_netcdfmemoryview→bytes,decode_timedelta=True._search_pages,get_root_catalog_cid(catalog_url), conftest probes); upstream's urllib3MaxRetryError.reasonrefinement dropped as structurally moot on httpx (HTTPStatusErroris not aTransportError, so exhausted status-retries already take the HAMT fallback — verified against installed py-hamt's retry re-raise surface).Compatibility notes
resolve_cid_from_stac_server/resolve_dataset_cid_from_stacreturnResolvedDatasetinstead ofstr;variant=""is now an explicit (unresolvable) variant rather than no-variant.httpx.HTTPErrorfamily) — callers catchingrequests.RequestExceptionmust update.metadata["variant"]now reports the actually-selected variant; consumers keying caches on the old always-"default"value will see the true names.How this was verified
read_textfallback, eager XOR validation, meta-test hardening).pytest232 passed / 45 skipped,mypy0 errors (16 files),ruff check+format --checkclean, lazy-import smoke intact.🤖 Generated with Claude Code