refactor: enforce cell_ids parameter in session serialization and caching#8904
refactor: enforce cell_ids parameter in session serialization and caching#8904
Conversation
…hing - Updated `serialize_session_snapshot` and `persist_session_view_to_cache` functions to require `cell_ids` as a mandatory parameter. - Enhanced `CachingExtension` to include the `document` parameter in `SessionCacheManager`. - Adjusted `SessionCacheWriter` to utilize `cell_ids` from the `NotebookDocument` for improved session management. - Updated tests to reflect changes in the serialization process, ensuring compatibility with the new mandatory `cell_ids` parameter.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR makes session snapshot serialization deterministic by requiring callers to pass an explicit cell_ids ordering, and threads the canonical NotebookDocument into session caching so the cache writer can always serialize cells in document order.
Changes:
- Make
serialize_session_view,serialize_session_snapshot, andpersist_session_view_to_cacherequirecell_ids(no implicit fallback ordering). - Update session caching to pass
NotebookDocumentintoSessionCacheManager/SessionCacheWriterand serialize usingdocument.cell_ids. - Update session serialization/caching tests to construct documents and pass
cell_idsexplicitly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/_session/state/test_serialize_session_missing_type.py | Updates test to use CellId_t and pass required cell_ids into serialize_session_view. |
| tests/_session/state/test_serialize_session.py | Refactors tests with helpers, adds NotebookDocument fixtures, and updates cache writer/manager construction and serialization calls. |
| marimo/_session/state/serialize.py | Makes cell_ids required and uses document.cell_ids for cache writes; updates cache writer/manager APIs to accept NotebookDocument. |
| marimo/_session/extensions/extensions.py | Passes session.document into SessionCacheManager during attach. |
| marimo/_server/export/_session_cache.py | Makes cell_ids required for snapshot serialization and persistence helpers. |
| When `cell_ids` is provided, it determines the order of the cells in | ||
| the NotebookSession schema (and only these cells will be saved to the | ||
| schema). When not provided, this method attempts to recover the notebook | ||
| order from the SessionView object, but this is not always possible. | ||
| """ |
There was a problem hiding this comment.
The docstring still describes cell_ids as optional ("When not provided...") but cell_ids is now a required parameter. Please update/remove that portion of the docstring so it matches the new API contract (and avoids suggesting a fallback behavior that no longer exists).
dmadisetti
left a comment
dmadisetti
left a comment
There was a problem hiding this comment.
I supposed there's never a case where cell_id != view.cell_id ? Looks good with passing tests
Unlikely. It's possible the view has a “leak” in edit mode and doesn't clean up notifications in some cases but that's ok - source of truth is now the document. |
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.21.2-dev81 |
serialize_session_snapshotandpersist_session_view_to_cachefunctions to requirecell_idsas a mandatory parameter.CachingExtensionto include thedocumentparameter inSessionCacheManager.SessionCacheWriterto utilizecell_idsfrom theNotebookDocumentfor improved session management.cell_idsparameter.