Skip to content

Preserve caller-owned protocol options when merging overrides - #2279

Merged
martindurant merged 2 commits into
fsspec:masterfrom
adenzhou1350:codex/fix-protocol-options-mutation
Oct 14, 2026
Merged

martindurant merged 2 commits into
fsspec:masterfrom
adenzhou1350:codex/fix-protocol-options-mutation

Conversation

@adenzhou1350

Copy link
Copy Markdown
Contributor

_un_chain copies the top-level kwargs, but takes the protocol-specific dictionary by reference. Merging per-call overrides with kws.update(kwargs) then changes the caller's original options. Reusing them in another call unexpectedly keeps the previous override.

For example, url_to_fs(url, file=options, auto_mkdir=True) changes options['auto_mkdir'] even when it was originally false. The same issue affects open and get_fs_token_paths, including the outer filesystem of chained URLs.

Copy the protocol-options dictionary before merging. This is intentionally shallow: credential/configuration objects keep their identity, and explicit keyword precedence is unchanged. The tests exercise all three public entry points with plain memory URLs and filecache/memory chains, then reuse the same caller-owned options.

Validation against master 94afb3f6:

  • Windows Python 3.14 core/config tests: baseline 6 failed, 62 passed, 6 skipped; candidate 68 passed, 6 skipped.
  • Linux Python 3.12, same test files: baseline 6 failed, 72 passed, 6 skipped; candidate 78 passed, 6 skipped. The registered compression cases differ by installed optional codecs. All six new regression cases fail before and pass after on both systems.
  • Native source exports use raw Git blobs. Both arms run the identical candidate test file; the baseline test-only overlay is recorded separately. Actual Linux package import paths and production module hashes were checked.
  • Diff check passes. Lint, full upstream CI and cloud backend integrations were not run locally. No network/cloud access is needed by the regression tests.

AI-assisted; independently reproduced through the real public filesystem constructors, without mocked imports or replacement implementations.

Signed-off-by: Xucheng Zhou <aden1350@outlook.com>
@adenzhou1350

Copy link
Copy Markdown
Contributor Author

GitHub currently shows one workflow awaiting maintainer approval here and on #2278, so upstream CI has not run for either PR. Could a maintainer approve those workflows when convenient? The before/after Windows and Linux regression results and their limits are recorded in the respective descriptions.

@adenzhou1350

Copy link
Copy Markdown
Contributor Author

Remaining CI authorization queue: #2257, #2258, #2259, #2261, #2262 and this PR each still show one workflow awaiting maintainer approval. Could a maintainer approve these runs together when convenient? They have not executed upstream CI yet; this is one combined request to avoid notifying on all six threads.

@martindurant

Copy link
Copy Markdown
Member

Could a maintainer approve these runs together when convenient?

I am catching up slowly. The rate of PR submissions in the past weeks has been truly unprecedented.

I also encourage the code contributors to review one-anothers' PRs before I get to them to reduce friction as much as possible.

@martindurant
martindurant merged commit 46af0f4 into fsspec:master Oct 14, 2026
11 checks passed
@adenzhou1350

Copy link
Copy Markdown
Contributor Author

Thanks for catching up and merging this, and for explaining the review backlog. Understood on peer review; I will keep follow-ups consolidated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants