refactor(framework): Add FFS-replacement methods to LinkState/NodeState#6809
refactor(framework): Add FFS-replacement methods to LinkState/NodeState#6809danieljanes wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds first-class FAB (Flower App Bundle) storage and retrieval APIs to the state layer (LinkState/NodeState) as a replacement for the previous FFS-style handling, including persistence via a new SQL table and migration.
Changes:
- Introduces
store_fab/get_fababstract methods onLinkStateandNodeStateand implements them for in-memory and SQL-backed state. - Adds a new
fabtable to the LinkState SQL schema plus an Alembic migration to create it. - Extends existing state test suites to cover FAB storage, deduplication-by-hash, and missing-hash behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| framework/py/flwr/supernode/nodestate/nodestate_test.py | Adds NodeState tests for storing/retrieving FABs, deduplication by content hash, and missing lookups. |
| framework/py/flwr/supernode/nodestate/nodestate.py | Extends the NodeState interface with store_fab/get_fab. |
| framework/py/flwr/supernode/nodestate/in_memory_nodestate.py | Implements FAB storage in the in-memory NodeState backend keyed by SHA-256(content). |
| framework/py/flwr/supercore/state/schema/linkstate_tables.py | Adds the fab table to LinkState SQLAlchemy metadata. |
| framework/py/flwr/supercore/state/alembic/versions/rev_2026_03_21_add_fab_table.py | Introduces an Alembic migration to create/drop the fab table. |
| framework/py/flwr/supercore/state/alembic/utils_test.py | Adds a migration test asserting the fab table exists after running migrations. |
| framework/py/flwr/server/superlink/linkstate/sql_linkstate.py | Implements FAB persistence in SqlLinkState (upsert + retrieval). |
| framework/py/flwr/server/superlink/linkstate/linkstate_test.py | Adds LinkState tests for FAB store/get, deduplication, and missing hash behavior. |
| framework/py/flwr/server/superlink/linkstate/linkstate.py | Extends the LinkState interface with store_fab/get_fab. |
| framework/py/flwr/server/superlink/linkstate/in_memory_linkstate.py | Implements FAB storage in the in-memory LinkState backend keyed by SHA-256(content). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Store a FAB and return its canonical SHA-256 hash.""" | ||
|
|
||
| @abstractmethod | ||
| def get_fab(self, fab_hash: str) -> Fab | None: | ||
| """Return the FAB for the given hash, if present.""" | ||
|
|
There was a problem hiding this comment.
store_fab accepts a Fab (which already contains hash_str), but the contract here doesn’t clarify whether implementations must trust fab.hash_str or recompute from fab.content. Current implementations recompute and effectively ignore the input hash_str, which can silently mask mismatches/bugs in callers. Consider either (a) validating fab.hash_str when non-empty (raise if it doesn’t match the SHA-256 of content) or (b) documenting explicitly that hash_str is ignored/overwritten (or changing the signature to accept content+verifications like the old FFS API).
| """Store a FAB and return its canonical SHA-256 hash.""" | |
| @abstractmethod | |
| def get_fab(self, fab_hash: str) -> Fab | None: | |
| """Return the FAB for the given hash, if present.""" | |
| """Store a FAB and return its canonical SHA-256 hash. | |
| Implementations **must** compute the canonical SHA-256 hash from | |
| ``fab.content`` and use that value as the authoritative identifier. | |
| Any value already present in ``fab.hash_str`` when this method is | |
| called is ignored and may be overwritten by the implementation. | |
| The returned string is this canonical SHA-256 hash and can be used | |
| later to retrieve the same FAB via :meth:`get_fab`. | |
| """ | |
| @abstractmethod | |
| def get_fab(self, fab_hash: str) -> Fab | None: | |
| """Return the FAB for the given hash, if present. | |
| Parameters | |
| ---------- | |
| fab_hash : str | |
| The canonical SHA-256 hash previously returned by | |
| :meth:`store_fab`. | |
| Returns | |
| ------- | |
| Optional[Fab] | |
| The stored FAB corresponding to ``fab_hash``, if present. The | |
| returned FAB's ``hash_str`` field, if set, should equal | |
| ``fab_hash``. | |
| """ |
| """Store a FAB and return its canonical SHA-256 hash.""" | ||
|
|
||
| @abc.abstractmethod | ||
| def get_fab(self, fab_hash: str) -> Fab | None: | ||
| """Return the FAB for the given hash, if present.""" | ||
|
|
There was a problem hiding this comment.
Same API ambiguity as in NodeState: store_fab takes a Fab that includes hash_str, but the contract doesn’t state whether hash_str must match sha256(content). Since implementations recompute the hash, callers can pass inconsistent Fab objects without noticing. Please either validate the provided hash_str (when set) or document that it’s ignored/overwritten (or change the signature to content+verifications).
| """Store a FAB and return its canonical SHA-256 hash.""" | |
| @abc.abstractmethod | |
| def get_fab(self, fab_hash: str) -> Fab | None: | |
| """Return the FAB for the given hash, if present.""" | |
| """Store a FAB and return its canonical SHA-256 hash. | |
| The canonical hash MUST be computed by the implementation from the FAB's | |
| content (for example, from its serialized representation) using SHA-256. | |
| The ``hash_str`` field on the passed ``fab`` argument, if present, MUST NOT | |
| be trusted by implementations. Implementations SHOULD ignore any existing | |
| value and MAY overwrite it with the canonical hash. Callers MUST NOT rely | |
| on ``fab.hash_str`` being consumed as input; instead, they MUST use the | |
| returned string as the authoritative hash. | |
| """ | |
| @abc.abstractmethod | |
| def get_fab(self, fab_hash: str) -> Fab | None: | |
| """Return the FAB for the given canonical hash, if present. | |
| The ``fab_hash`` parameter MUST be the canonical SHA-256 hash previously | |
| returned by :meth:`store_fab`. | |
| """ |
This PR:
store_fabandget_fab)