feat: versioned objects and migrations for syft-client#9464
feat: versioned objects and migrations for syft-client#9464bitsofsteve wants to merge 12 commits into
Conversation
pjwerneck
left a comment
There was a problem hiding this comment.
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
VersionInfosays 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 aVersionInfoV3with a new required field, breaking old readers. I'd add a hard check, like a__init_subclass__method that verifies newVersionInfosubclasses on creation.
|
|
||
| # 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() |
There was a problem hiding this comment.
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()There was a problem hiding this comment.
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.
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) andFileChangeEventsMessage: envelopes are the migratable units; items stay unregistered so checkpoint/cache payloads are unchanged.PeerManager.live_peer_schemas(protocol)feeds live peer schemas intoJobStorage/DatasetStorage: jobs negotiate per peer, datasets per audience. Also fixespeer_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.