Skip to content

fix: keep flytekit on s3fs/gcsfs/adlfs when another library registers a different fsspec implementation - #3472

Merged
AdilFayyaz merged 1 commit into
masterfrom
adil/fsspec-canonical-impl
Oct 2, 2026
Merged

AdilFayyaz merged 1 commit into
masterfrom
adil/fsspec-canonical-impl

Conversation

@AdilFayyaz

Copy link
Copy Markdown
Collaborator

Why are the changes needed?

Importing a library that registers its own fsspec implementation for s3/gs/abfs with clobber=True (e.g. the Flyte 2 SDK, flyte, which registers obstore) silently replaces the filesystem flytekit uses. obstore ignores the call kwargs flytekit relies on. On GCS, this means deck.html is stored without content_type="text/html", so the console downloads the deck instead of rendering it. On S3, the upload fails outright, because obstore rejects s3fs constructor kwargs such as cache_regions.

What changes were proposed in this pull request?

  • Add _canonical_fs_class(protocol) in flytekit/core/data_persistence.py. It resolves fsspec's default implementation for the protocol from known_implementations. A
    registered implementation is used only if it subclasses that default (as flytekitplugins-async-fsspec's AsyncS3FileSystem does).
  • FileAccessProvider.get_filesystem uses it for s3, gs, abfs and abfss; every other protocol is unchanged. This restores ContentType/content_type, the S3 metadata and
    chunksize handling for every flytekit upload and download, not just decks.
  • No behaviour change when nothing else registers these protocols.

How was this patch tested?

  • New unit tests in tests/flytekit/unit/core/test_data_persistence.py:
    • an unrelated registered s3/gs implementation is ignored;
    • a registered subclass of S3FileSystem is kept;
    • put_data passes ContentType (s3) and content_type (gs) through to s3fs/gcsfs with an unrelated implementation registered. These fail on master and pass with this
      change.
  • pytest tests/flytekit/unit/core/test_data_persistence.py -m "not sandbox_test" → 17 passed.
  • Manual round trip against a local S3 (moto), with flyte 2.10.6 and obstore 0.7.3 to 0.11.1 installed:
    • import flyte.storage, then FileAccessProvider.put_data(deck.html, ..., ContentType="text/html"), then read the object's metadata back with head_object.
    • master: the upload fails with UnknownConfigurationKeyError: 'cache_regions'.
    • This branch: the object is written via S3FileSystem with ContentType: text/html.

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

Signed-off-by: M. Adil Fayyaz <62440954+AdilFayyaz@users.noreply.github.com>
@codecov

codecov Bot commented Oct 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.58%. Comparing base (d69b3fb) to head (b6f3d88).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3472      +/-   ##
==========================================
- Coverage   86.67%   83.58%   -3.09%     
==========================================
  Files         443        3     -440     
  Lines       37040      195   -36845     
  Branches     3036        0    -3036     
==========================================
- Hits        32106      163   -31943     
+ Misses       4052       32    -4020     
+ Partials      882        0     -882     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AdilFayyaz
AdilFayyaz merged commit eb22787 into master Oct 2, 2026
57 of 58 checks passed
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