Skip to content

feat: versioned DatasetStorage for reading/writing datasets across protocol versions#9443

Open
koenvanderveen wants to merge 4 commits into
devfrom
koen/dataset-storage
Open

feat: versioned DatasetStorage for reading/writing datasets across protocol versions#9443
koenvanderveen wants to merge 4 commits into
devfrom
koen/dataset-storage

Conversation

@koenvanderveen

Copy link
Copy Markdown
Collaborator

Summary

Mirrors the syft-job migration/storage architecture (PR #9434) in syft-datasets, adding a versioned read/write layer so datasets can evolve their on-disk format while staying compatible with older peers.

  • DatasetStorage — the single choke point for all dataset filesystem IO. Reads select a ProtocolCodec by the on-disk protocol version and upgrade the object to the latest version in memory; writes downgrade to the target protocol and let the codec persist it. Nests a MigrationRegistry, peer_schemas, and codecs.
  • v<n> path segment — new datasets can live under public/syft_datasets/v<n>/<name>/ (and private/…). Protocol 0 = the last release (syft-client 0.1.117 / syft-dataset 0.1.20): flat layout, no v<n>, no canonical_name/version fields — read and written back byte-compatibly.
  • Versioned modelsDataset / PrivateDatasetConfig are now MigratableObjects under models/<object>/v1.py. Object schemas are unchanged since 0.1.117, so only v1 exists; the protocol bump is purely a path-layout + identity-field change. dataset.py is kept as a re-export shim.
  • DatasetRef — path-derived identity (owner, name, protocol_version); the dataset name comes from the path, not the spoofable file.
  • Broadcast write — a public dataset is a single copy read by the whole audience (unlike jobs, which target one peer), so the write protocol(s) come from the audience's versions (target_protocol_versions_for_peers), defaulting to the widest-compatible (oldest) protocol when no peers are known. peer_schemas is injectable but not yet populated by syft-client, so today datasets are written in protocol 0 and the v1 layout ships dormant (writing v1 now would hide datasets from 0.1.117 peers).
  • SyftDatasetManager routes all reads/writes through DatasetStorage and supports multi-version write (one on-disk copy per protocol version in the audience); get_all dedupes copies across layouts, preferring the newest protocol.
  • Release toolingscripts/export_release_artifact.py + generate_release_fixture.py; the protocol-0 / 0.1.20 artifacts and fixtures are derived as if that release had emitted them.
  • Tests mirror syft-job/tests/migrations (unit + p2p), including a byte-exact cross-release round-trip over hand-generated fixtures and a multi-version-write test. CI (unit-tests.yml dataset-tests job) and justfile (test-unit-datasets) wired.

Scope

Package core only, per discussion. syft-client wiring (advertising each peer's dataset protocol version and writing in it at create/share) is a follow-up — syft-client currently exchanges only a single client-version string, not per-package schemas.

Testing

  • uv run pytest packages/syft-datasets/tests40 passed
  • just test-unit-fast → 406 passed
  • tests/unit dataset + sync_manager + delete suites → green
  • pre-commit run --all-files → clean

…otocol versions

Mirror the syft-job migration/storage architecture in syft-datasets:

- DatasetStorage: single choke point for all dataset filesystem IO. Reads select
  a ProtocolCodec by on-disk protocol version and upgrade to the latest object
  version in memory; writes downgrade to the target protocol and the codec
  persists it. Nests a MigrationRegistry, peer_schemas, and codecs.
- On-disk protocol version: new datasets can live under a v<n> path segment
  (public/syft_datasets/v<n>/<name>/). Protocol 0 = the last release
  (syft-client 0.1.117 / syft-dataset 0.1.20): flat layout, no v<n>, no
  canonical_name/version fields; read + written back byte-compatibly.
- Versioned models: Dataset / PrivateDatasetConfig are now MigratableObjects
  under models/<object>/v1.py. Object schemas are unchanged since 0.1.117, so
  only v1 exists; the protocol bump is purely path-layout + identity fields.
- DatasetRef: path-derived identity (owner, name, protocol_version).
- Broadcast write: a public dataset is one copy read by the whole audience, so
  the write protocol(s) come from the audience's versions
  (target_protocol_versions_for_peers), defaulting to the widest-compatible
  (oldest) protocol when no peers are known. peer_schemas is injectable but not
  yet populated by syft-client, so today datasets are written in protocol 0 and
  the v1 layout ships dormant.
- SyftDatasetManager routes all reads/writes through DatasetStorage and supports
  multi-version write (one copy per protocol version in the audience); get_all
  dedupes copies across layouts preferring the newest protocol.
- migrations/history + release scripts (export_release_artifact.py,
  generate_release_fixture.py); protocol-0/0.1.20 artifacts and fixtures derived
  as if that release had emitted them.
- Tests mirror syft-job/tests/migrations (unit + p2p, incl. byte-exact
  cross-release round-trip and a multi-version-write test); CI + justfile wired.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The enclave inference example computed private dataset dirs using the
default protocol version (v1 segment), but datasets shared with 'any'
are written at the widest-compatible protocol (0, no segment). This
mismatch broke the log-sink path and the ensure_logs_dataset idempotency
check. Resolve the actual on-disk ref via DatasetStorage, falling back to
the widest-compatible protocol for datasets not yet synced.
Comment thread packages/syft-datasets/pyproject.toml
)


def _seed_syftbox(syftbox: Path, src_dir: Path, protocol_version: str) -> None:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double check this script

package-artifacts/ and protocols/ hold released history only. Only 0.1.20
(protocol 0) shipped to PyPI, so the frozen 0.1.21/protocol-1 artifacts were
premature. The unreleased protocol 1 already lives in the live registry and
will be frozen by export_release_artifact.py at release time (mirrors syft-job).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
def public_dir(self) -> Path:
return self.syftbox_folder / self.email / "public"

def public_datasets_root_for_datasite(self, datasite: str) -> Path:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it feels like this should be part of the codec

Comment thread packages/syft-datasets/src/syft_datasets/dataset.py
Comment thread packages/syft-datasets/src/syft_datasets/dataset_manager.py
Comment thread packages/syft-datasets/src/syft_datasets/dataset_manager.py
Comment thread packages/syft-datasets/src/syft_datasets/dataset_manager.py
Comment thread packages/syft-datasets/src/syft_datasets/dataset_manager.py
Comment thread packages/syft-datasets/src/syft_datasets/dataset_manager.py
@@ -166,86 +136,106 @@ def create(
users: list[str] | str | None = None,
# copy_private_data: bool = True, # TODO

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps we should also have the posibility to pass in protocol verisons here in case we want to override something

codec.read(path, "PrivateDatasetConfig"), "PrivateDatasetConfig"
)

def write_dataset(self, ref: DatasetRef, dataset: Dataset) -> Path:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably for all the writes of datasets, we first want to see which versions are already on disk. If we have multiple versions on disk already we need to make sure that they are in sync. So we first need a step that checks the existing protocols on disk, then finds the codecs for those protocols (and add the ref protocol) and then write for all codecs

peer_emails = None if users in (None, SHARE_WITH_ANY) else list(users)
return self.storage.target_protocol_versions_for_peers(peer_emails)

def _create_one_version(

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the responsibility of this should probably lie with the storage layer

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@pjwerneck pjwerneck left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few issues found and commented on.

Overall, the high-level design is in line with #9434, but the fact peer_schemas is still not properly wired is a bit concerning. In production, everything still resolves to v0. I'd be more comfortable if v1 and the whole protocol mechanics was fully integrated and tested end-to-end before wiring it to the rest of syft-client.

for ref in best_ref.values():
try:
all_datasets.append(self.storage.read_dataset(ref))
except Exception:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blanket Exception catch with no logging. This will silently ignore errors due to unreadable datasets in read_dataset. Not minor issue, since migration failures or schema inconsistencies can make the dataset vanish with no logs.

dataset_uid = uuid4()

created: dict[str, Dataset] = {}
for protocol_version in sorted(target_versions, key=int):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can create a dataset in several protocol versions at once, but delete() uses get() which uses store.find_dataset_ref(), which returns only a single dataset, with the newest protocol. It's going to leave older copies. Is that intended? If not, delete() should iterate all versions too.


@property
def owner(self) -> str:
return self.mock_url.host

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the canonical owner come from the path?

private_path: Path = to_path(private_path)
readme_path: Path | None = to_path(readme_path) if readme_path else None
# Reject names that collide with a protocol-version directory (v1, v2, ...).
# (The stricter _validate_dataset_name regex is intentionally not enforced

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, SyftDatasetManager._validate_dataset_name is dead code and should be removed.

)

def private_dataset_dir(self, ref: DatasetRef) -> Path:
return self.config.get_private_dataset_dir(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both codecs (v0 and v1) are deferring the public and private paths to config. Wasn't the point of the refactoring to have the codec own disk layout?

config.model_dump(mode="json"), indent=2, sort_keys=False
).encode()
data = config.disk_dict()
if ref.protocol_version == "0":

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be handled by the codec, not by the manager. The manager should never make internal decisions based on protocol version.

data = codec.read(codec.metadata_path(ref), "Dataset")
dataset = self._upgrade(data, "Dataset")
# Name comes from the path (the dataset dir), never a spoofable file.
dataset.name = ref.name

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the counterpart to the comment on owner property in models/dataset/v1.py:69. If we assign the name from ref.name here, why not the owner too?

from pydantic import BaseModel, Field
from pathlib import Path
from .url import SyftBoxURL
from .migrations.registry import DATASET_PROTOCOL_VERSION

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The config.py now has a protocol_version parameter everywhere, which defaults to this imported DATASET_PROTOCOL_VERSION,the current protocol, but the storage defaults to the widest. Is that intended?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants