Skip to content

feat: versioned objects and migrations for syft-client#9464

Draft
bitsofsteve wants to merge 12 commits into
devfrom
stephen/syft-client-migrations
Draft

feat: versioned objects and migrations for syft-client#9464
bitsofsteve wants to merge 12 commits into
devfrom
stephen/syft-client-migrations

Conversation

@bitsofsteve

Copy link
Copy Markdown
Member

Versioned objects for syft-client with migrations, plus peer protocol-schema exchange wired into JobStorage and DatasetStorage.

  • VersionInfo (SYFT_version.json): V1 + V2. V2 advertises slim protocol schemas for syft-client/syft-job/syft-dataset; first migrations (v1<->v2).
  • ProposedFileChangesMessage (msgv2) and FileChangeEventsMessage: envelopes are the migratable units; items stay unregistered so checkpoint/cache payloads are unchanged.
  • Protocol 0 = releases <= 0.1.117 (no identity fields); reads upgrade in memory via shims at the existing choke points. Old clients ignore the new keys (verified against the 0.1.117 models).
  • Release discipline: history artifacts for 0.1.117, export script with bump/frozen-once guards, blob fixtures + p2p compat tests.
  • PeerManager.live_peer_schemas(protocol) feeds live peer schemas into JobStorage/DatasetStorage: jobs negotiate per peer, datasets per audience. Also fixes peer_schemas or {} dropping the shared reference in both storages.

Out of scope: checkpoints, Peer/SYFT_peers.json, local caches, the semver handshake (unchanged).

One wave per commit; green at every commit.

@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.

No major issues found. Excellent PR.

Some high-level concerns:

  • The PeerManager - storage coupling via a shared mutable dict is fragile, and apparently already caused a bug fixed in this PR (the peer_schemas or {} check). The assumption that the dict must not be copied is an unwritten contract easy to overlook.
  • Every peer of every version must be capable of parsing every future version file, and VersionInfo says changes must be additive-only, but there's nothing enforcing that. It's a convention that everything new must be optional, but there's nothing preventing someone from adding a VersionInfoV3 with a new required field, breaking old readers. I'd add a hard check, like a __init_subclass__ method that verifies new VersionInfo subclasses on creation.

Comment thread syft_client/__init__.py

# Historic schemas list object versions that must already be registered, which
# happens when the model modules are imported (transitively via sync.login).
register_historic_schemas()

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 call is relying on an implicit import order of the versioned classes. Even though that's documented in register_historic_schemas, I'd make those imports explicit here, so it looks intentional and not a side-effect.

# Ensure versioned models are registered
import syft_client.sync.version.version_info      # noqa: F401, E402
import syft_client.sync.messages.proposed_filechange  # noqa: F401, E402
import syft_client.sync.events.file_change_event  # noqa: F401, E402
register_historic_schemas()

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.

Minor, but this script is checking protocol first, then writing the package artifact first. If an error happens between the two writes, you're left with the package artifact written, and on a re-run the info_path.exists() exits, blocking the second protocol write attempt, requiring a manual recovery. There should be a single exit point when both paths are confirmed to exist.

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