Per-source client_config registry on Catalog - #1886
Draft
shcheklein wants to merge 11 commits into
Draft
Conversation
Storage credentials were a single dict per Catalog, so a per-call client_config on read_storage could only take effect by swapping the whole catalog: Session.get silently created a side session and entered it as the ambient context, changing the behavior of every later bare call in the process. Replace that with a per-source registry on the catalog itself: - Catalog.source_client_configs maps a storage root (s3://bucket, file:///dir) to its config; read_storage registers the per-call config (including auto-detected anon) under the canonical listing URI, so the key always matches the `source` of listed files. - Catalog.get_client / client_config_for resolve: explicit per-call kwargs > registered per-source config > catalog default. - get_init_params ships the registry to parallel/distributed UDF workers and the PyTorch loader, so files materialized in workers resolve their per-source config (in-process part of #1778). - Session.get no longer forks on config mismatch; different sources with different configs coexist in one session, and the same source with two different explicit configs raises with a remedy (an auto-anon entry may be upgraded by an explicit config). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Deploying datachain with
|
| Latest commit: |
0953dc6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1e89607b.datachain-2g6.pages.dev |
| Branch Preview URL: | https://per-source-client-config.datachain-2g6.pages.dev |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace parse-derived storage-root keys with plain URI prefixes: registration stores the listed URI as-is (rstripped), lookups resolve the longest registered prefix of the object's full URI. This removes the Client.parse_url coupling, the canonical-form dependency on get_listing (registration now happens before it, and get_listing's client_config parameter is gone again), and the file-vs-directory dual-parse fallback. It is also strictly more capable: two subtrees of one bucket can carry different configs (nested prefixes allowed, longest wins), which parse-to-bucket keys rejected as conflicts. Conflict detection now applies only to re-registering the same prefix. Resolution sites pass the file's full URI (File._client/_full_uri, query/schema.py params, zarr), so subtree-scoped configs reach every read path, including UDF workers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
File reads resolve their config via client_config_for_file(source, path) instead of building a full URI and scanning the registry per row: the registry view is memoized per source (invalidated on registration), so resolving a file costs one dict lookup plus a str.startswith on the path only when subtree prefixes are registered under its source. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Key the registry by File.source (bucket for cloud, directory for local) instead of arbitrary URI prefixes. read_storage derives the key by parsing the canonical listing URI, so registration and the sources carried by listed files agree exactly. This deletes the nested-prefix feature and everything it required: longest-match ordering, length sorting, the per-source memo, and the per-row (source, path) resolution. File reads resolve with a single dict lookup on file.source, and get_client's existing fallback covers it, so File and UDF-parameter code paths revert to main's original form. Two subtrees of one bucket with different configs is a conflict error again (remedy: an explicit Session); that capability can return as a follow-up if needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
client_config_for is now a plain dict lookup by source — the covering startswith scan (and its path-boundary subtlety) is gone. The two call sites that passed sub-source URIs now pass the parsed source instead: read_storage looks up by the source it just derived from the listing URI, and get_listing's fallback parses the storage root of its uri. Destination writes no longer implicitly resolve a registered source's config; as on main, they take credentials via their explicit client_config parameter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
File.export built its destination client from the full output path, so with exact-match lookup it fell back to the catalog default even when the destination's source had a registered config — while the actual write (save -> _resolve_destination) resolved the registry via the parsed storage root. Reuse _resolve_destination for the export-side client too, so both, including the makedirs call, consistently use the config registered for the destination's source (explicit per-call config still wins). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two fixes for failures the conflict rule surfaced on CI:
- A derived {"anon": True} registry entry now overlays the catalog
default at resolution instead of replacing it. Registering bare anon
dropped defaults like the GCS test server's endpoint_url, so file
access talked to the real endpoint and 404ed (all gs-parametrized
jobs). This mirrors main, which merged detected anon into the
session default. An explicitly registered config still replaces the
default.
- Normalize `anon` to a bool at the read_storage boundary. Callers
pass truthy strings ("True" in the get_started examples); with the
value participating in registry equality, {"anon": "True"} vs
{"anon": True} for the same bucket raised a false conflict
(examples jobs). bool() matches the effective truthiness semantics
these values already had.
Verified end-to-end: examples/get_started/json-csv-reader.py (the
failing example) runs clean against the public bucket.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The single-file probe in get_listing received the bare detected
{"anon": True} instead of the default-refined view, so with an
endpoint-style default (the gs test server) it probed the wrong
endpoint, silently misclassified files as directories, and listing
failed with "Unable to resolve remote path" (remaining gs CI
failures). Refine the config passed to get_listing the same way
Catalog.client_config_for refines a registered bare-anon entry.
Directory listings passed by accident: their probe returns False
either way.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The upgrade rule covered marker -> explicit but not the reverse: after
an explicit config was registered for a source (e.g. a test fixture's
endpoint config threaded through dataset creation), a later bare read
that auto-detected anon tried to register the {"anon": True} marker
and raised the conflict error. A derived guess now yields to whatever
is already registered.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
What & why
Storage credentials (
client_config) were a single dict perCatalog, and everything resolves clients through the catalog — so a per-callclient_configonread_storage()could only take effect by swapping the whole catalog.Session.getdid that by silently creating a side session and entering it as the ambient context, changing the behavior of every later bare call in the process (action at a distance), and the config was lost entirely when an explicit session was passed (UDF reads on such chains failed with 403).This PR decouples credentials from session identity with a per-source config registry on the catalog:
Catalog.source_client_configs: {source → config}— keys areFile.sourcevalues (the bucket for cloud storage, the directory for local paths).read_storageregisters its per-call config (including the auto-detectedanon=True) under the source of each listing; registration and the sources carried by listed files agree exactly because both come from the same listing URI.file.source, and the listing paths look up by the source they parse from the listing URI. No prefix matching anywhere.get_init_params()ships the registry to parallel/distributed UDF workers and the PyTorch loader, so files materialized in workers resolve their source's config — including Studio's distributed path, which serializes the samecatalog_initpayload. This fixes the in-process part of anon does not propagate across .save() boundaries #1778 (anonnot propagating across.save()boundaries).Session.getno longer forks on config mismatch — the branch is deleted.Behavior changes
read_storage(client_config=…)no longer creates/enters a hidden session; later bare calls are unaffected.unionof two buckets, or of separately-listed local directories, works; each file resolves the config of the listing that produced it).ValueErrornaming the source, with the remedy (pass an explicit Session(client_config=...)). An auto-anon entry ({"anon": True}) may be upgraded by an explicit config. Two subtrees of one cloud bucket with different configs share a source and therefore conflict — per-prefix credentials are out of scope (possible follow-up).chain.session.catalog.client_configno longer reflects per-call config; usecatalog.client_config_for(uri).to_storage,file.export/save) with no explicit config resolve the config registered for the destination's source (derived by parsing the destination; exact match), else the catalog default.read_storage(session=explicit, client_config=…)now registers the config on that session's catalog (previously discarded and re-applied to the listing only — file reads inside UDFs on such chains used the wrong credentials).Out of scope (follow-up to #1778): persisting config across processes on saved datasets — credentials don't belong in dataset metadata.
Tests
tests/unit/test_client_config_registry.py: source-key normalization, exact-match lookup, distinct sources coexisting, same-source conflict + auto-anon upgrade, defensive copy,get_clientprecedence, init-params shipping.tests/func/test_client_config_registry.py: no-session-fork regression, same-source conflict error, two-sources-two-configs union, config-follows-the-listing-source semantics, worker propagation (parallel=2, asserts resolution happens in a worker process), same-processsave()→read_dataset()resolution.test_storage_auto_anon.py/test_datachain.pyassertions moved fromsession.catalog.client_configtocatalog.client_config_for(uri)and now also pin that the session-wide default stays untouched.🤖 Generated with Claude Code