feat: Implement support for multiple storage backends #99

FrithjofWinkelmannQC · 2025-07-17T14:07:33Z

Motivation

Closes #87.

This PR introduces fsspec to support different storage backends for IO methods. For now, we assume all relevant access parameters are configured via environment variables.

Changes

Added fsspec to dependencies and modified IO calls in collection.py to use that instead of the native python library

FrithjofWinkelmannQC · 2025-07-17T14:08:49Z

dataframely/collection.py

+        assert isinstance(fs, fsspec.AbstractFileSystem)
+        try:
+            fs.makedir(directory, create_parents=True)
+        except FileExistsError:


fs.makedir has no exists_ok parameter.

Did you try fs.makedirs? It seems to provide that parameter.

codecov · 2025-07-17T14:08:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (fdd8977) to head (60b6b6a).

Additional details and impacted files

@@            Coverage Diff            @@
##              main       #99   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           41        41           
  Lines         2401      2410    +9     
=========================================
+ Hits          2401      2410    +9

☔ View full report in Codecov by Sentry.
📢 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.

delsner

Nice progress, thanks!

delsner · 2025-07-17T16:02:25Z

dataframely/collection.py

+        fs, _ = fsspec.url_to_fs(
+            directory if isinstance(directory, str) else str(directory)
+        )


Suggested change

fs, _ = fsspec.url_to_fs(

directory if isinstance(directory, str) else str(directory)

)

fs, _ = fsspec.url_to_fs(str(directory))

Isn't this the same?

Yes, thanks for catching

delsner · 2025-07-17T16:06:07Z

dataframely/collection.py

+        assert isinstance(fs, fsspec.AbstractFileSystem)
+        try:
+            fs.makedir(directory, create_parents=True)
+        except FileExistsError:


Did you try fs.makedirs? It seems to provide that parameter.

delsner · 2025-07-17T16:17:31Z

dataframely/collection.py

+        except FileExistsError:
+            pass
+
+        with fs.open(f"{directory}{fs.sep}schema.json", "w") as f:


I think we need to ensure that directory doesn't already end with a separator here.

Done with directory = directory.rstrip(fs.sep)

delsner · 2025-07-17T16:18:14Z

dataframely/collection.py

+        if isinstance(directory, Path):
+            directory = str(directory)
+        fs, _ = fsspec.url_to_fs(directory)


Suggested change

if isinstance(directory, Path):

directory = str(directory)

fs, _ = fsspec.url_to_fs(directory)

fs, _ = fsspec.url_to_fs(str(directory))

delsner · 2025-07-17T16:18:51Z

dataframely/collection.py

        # First, we check whether the path provides the serialization of the collection.
        # If it does, we check whether it matches this collection. If it does, we assume
        # that the data adheres to the collection and we do not need to run validation.
-        if (json_serialization := directory / "schema.json").exists():
-            metadata = json_serialization.read_text()
+        if fs.exists(json_serialization := f"{directory}{fs.sep}schema.json"):


s.a. I think we need to make sure that directory doesn't already have a trailing path separator.

Same as above

delsner · 2025-07-17T16:19:21Z

pyproject.toml

 module = ["pyarrow.*", "pytest_benchmark.*", "sklearn.*"]

+[[tool.mypy.overrides]]
+module = ["fsspec.*"]
+follow_untyped_imports = true
+


Suggested change

module = ["pyarrow.*", "pytest_benchmark.*", "sklearn.*"]

[[tool.mypy.overrides]]

module = ["fsspec.*"]

follow_untyped_imports = true

module = ["fsspec.*", "pyarrow.*", "pytest_benchmark.*", "sklearn.*"]

delsner · 2025-07-17T16:21:46Z

pyproject.toml

I think we also need to add fsspec in pyproject.toml as a dependency.

FrithjofWinkelmannQC · 2025-07-18T15:58:10Z

.github/workflows/ci.yml

-        os: [ubuntu-latest, windows-latest]
+        os: [ubuntu-latest]
+        environment: [py310, py311, py312, py313]
+    services:


Services are only supported on linux OS in Github CI. I didnt find a way to dry, so I just created two jobs.

Out of curiosity: did you try using docker compose up minio -d --wait instead of using GitHub actions services? This might also work on Windows.

delsner · 2025-07-18T15:59:49Z

tests/collection/test_read_write_parquet.py

+
+def check_platform(path_fixture: str) -> None:
+    if platform.system() == "Windows" and path_fixture == "s3_path":
+        pytest.skip("Skipping because Minio is not set up in Windows CI")


I think we should just skip s3 tests if no s3 envvars are set to be able to run tests locally without starting minio.

That makes more sense, will adjust :)

delsner

Thanks, just a few smaller comments.

delsner · 2025-07-22T07:40:11Z

.github/workflows/ci.yml

@@ -32,14 +32,55 @@ jobs:
      - name: pre-commit
        run: pixi run pre-commit-run --color=always --show-diff-on-failure

-  unit-tests:
+  unit-tests-linux:
    name: Unit Tests (${{ matrix.os == 'ubuntu-latest' && 'Linux' || 'Windows' }}) - ${{ matrix.environment }}


Suggested change

name: Unit Tests (${{ matrix.os == 'ubuntu-latest' && 'Linux' || 'Windows' }}) - ${{ matrix.environment }}

name: Unit Tests (Linux) - ${{ matrix.environment }}

delsner · 2025-07-22T07:42:06Z

.github/workflows/ci.yml

-        os: [ubuntu-latest, windows-latest]
+        os: [ubuntu-latest]
+        environment: [py310, py311, py312, py313]
+    services:


Out of curiosity: did you try using docker compose up minio -d --wait instead of using GitHub actions services? This might also work on Windows.

delsner · 2025-07-22T07:42:41Z

.github/workflows/nightly.yml

@@ -14,13 +14,47 @@ permissions:

 jobs:
  unit-tests:
-    name: Unit Tests (${{ matrix.os == 'ubuntu-latest' && 'Linux' || 'Windows' }})
+    name: Unit Tests (Linuxs)


Suggested change

name: Unit Tests (Linuxs)

name: Unit Tests (Linux)

delsner · 2025-07-22T07:43:16Z

.github/workflows/ci.yml

@@ -32,14 +32,55 @@ jobs:
      - name: pre-commit
        run: pixi run pre-commit-run --color=always --show-diff-on-failure

-  unit-tests:
+  unit-tests-linux:
    name: Unit Tests (${{ matrix.os == 'ubuntu-latest' && 'Linux' || 'Windows' }}) - ${{ matrix.environment }}
    timeout-minutes: 30
    runs-on: ${{ matrix.os }}


Do we need the matrix here at all?

delsner · 2025-07-22T07:52:40Z

tests/collection/test_read_write_parquet.py

+def s3_path() -> str:
+    """Fixture to provide a mock S3 path for testing."""
+    s3 = boto3.resource("s3")
+    bucket_name = "".join(random.choice(string.ascii_lowercase) for _ in range(10))


We could just use a uuid here, wdyt?

delsner · 2025-07-22T07:56:58Z

tests/collection/test_read_write_parquet.py

Could we please also test that everything works if a directory path with trailing / is passed?

delsner · 2025-07-22T07:57:47Z

tests/collection/test_read_write_parquet.py

Does polars support anything else than s3:// paths beyond regular file paths? Should we test for those as well (e.g., file:// or hf://)?

AndreasAlbertQC · 2025-08-06T11:40:24Z

Hey @FrithjofWinkelmannQC, I wanted to check in with you on how we should proceed with this PR. A little background info: @borchero and I have been working towards integrating dataframely with deltalake, which we think is a generally useful storage backend to use. deltalake stores tables as parquet files and offers us some nice advantages over storing raw parquet files that we want to take advantage of (also outside of dataframely). To this end, I am currently preparing #109 , which would factor the storage specifics out of Schema and Collection and make it easier to add new storage types.

Anyway, my point is: If we have deltalake support, s3 support is included for free. Instead of .write_parquet, .read_parquet, it would then be .write_delta, .read_delta (see e.g. polars docs). For most use cases, this should be a transparent change to the parquet versions. What do you think? Happy to also chat about it offlien

FrithjofWinkelmannQC added 2 commits July 16, 2025 17:32

First implementation using fsspec

83e005d

Use fs.sep

4c2e1e3

FrithjofWinkelmannQC requested review from borchero, AndreasAlbertQC and delsner as code owners July 17, 2025 14:07

FrithjofWinkelmannQC marked this pull request as draft July 17, 2025 14:07

FrithjofWinkelmannQC commented Jul 17, 2025

View reviewed changes

delsner changed the title ~~Fsspec for storage backends~~ feat: Implement support for multiple storage backends Jul 17, 2025

github-actions bot added the enhancement New feature or request label Jul 17, 2025

delsner reviewed Jul 17, 2025

View reviewed changes

FrithjofWinkelmannQC added 3 commits July 18, 2025 17:39

Address reviewer comments, make S3 access work, update CI

e737b82

Test without windows

60b6b6a

Skip S3 tests on Windows

352f05f

FrithjofWinkelmannQC commented Jul 18, 2025

View reviewed changes

delsner reviewed Jul 18, 2025

View reviewed changes

Fix test name and nightly CI

f4ef143

FrithjofWinkelmannQC marked this pull request as ready for review July 18, 2025 16:04

Use environment variables for skipping tests

ca0b1db

delsner requested changes Jul 22, 2025

View reviewed changes

	name: Unit Tests (${{ matrix.os == 'ubuntu-latest' && 'Linux' \|\| 'Windows' }}) - ${{ matrix.environment }}
	name: Unit Tests (Linux) - ${{ matrix.environment }}

feat: Implement support for multiple storage backends #99

Are you sure you want to change the base?

feat: Implement support for multiple storage backends #99

Conversation

FrithjofWinkelmannQC commented Jul 17, 2025 • edited by delsner Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

delsner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delsner Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delsner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndreasAlbertQC commented Aug 6, 2025

Uh oh!

Uh oh!

FrithjofWinkelmannQC commented Jul 17, 2025 •

edited by delsner

Loading

codecov bot commented Jul 17, 2025 •

edited

Loading

delsner Jul 17, 2025 •

edited

Loading