Try to find a better API for notary - #455
Draft
Carreau wants to merge 5 commits into
Draft
Conversation
`compute_signature`, `mark_cells`, `_check_cell` and `check_cells` do not touch the signature store: they only need the notebook, and in one case the secret and the digest. Move their bodies to private module-level functions and have `NotebookNotary` delegate, so the logic is available without a notary. Pure refactor: no public name is added or removed, no behaviour changes, and the test suite is untouched. `check_cells` still dispatches through `self._check_cell` so subclasses overriding it keep working.
Signing needs an open signature store, and nothing in the API said whose
job it was to close it. Give that job to an object:
with notary.open_session() as session:
session.sign(nb)
A session owns the store it opens and closes it when the block exits, so
the store's lifetime is the session's lifetime. `open_session()` is a
context manager, so the session has no public `close()` -- it is not
something you can keep alive past the block. Each call opens its own
store, so sessions are independent: nestable, sequential, or per-thread.
The notary keeps its configuration (secret, algorithm, store_factory) and
becomes the thing that hands out sessions. Its own signing methods keep
working unchanged, delegating to a session over the shared store, and
still honour a subclass's compute_signature override.
`NotebookNotaryContext` stays the protocol it was in 5.11; both classes
satisfy it structurally. `TrustNotebookApp` signs through a session,
taking it as a required keyword-only argument, and docs/api.rst
documents the new API.
The notary opened its store in `__init__` and closed it in `close()`, which made a closed notary unusable and made `with notary:` a one-shot. Create the store lazily instead, and split the two kinds of close: - `close()` is the caller saying they are done. The store is closed and not re-created: the direct API then raises RuntimeError pointing at open_session(). Sessions are unaffected, since they own their stores. - `_close_store()` is the internal close, used by `__exit__`. It releases the store but leaves the notary working, so code that used the notary before it was a context manager keeps working after a block exits. A store assigned to `notary.store` by the caller belongs to the caller and is never closed by either path. Store creation takes a lock, and a failed `store.close()` puts the store back rather than dropping it. Because the store is no longer opened in `__init__`, `jupyter trust --reset` no longer opens the database it is about to delete, and `compute_signature`/`mark_cells`/`check_cells` open nothing at all.
Signing through the notary itself leaves its store open until someone remembers to call close(), which is what sessions exist to fix. Warn once per notary, both when the signing methods are called directly and when the notary is used as a context manager, and point at `open_session()` in both cases. The warning is a FutureWarning rather than a PendingDeprecationWarning: nothing here is scheduled for removal, and the notary's own methods stay supported, so the message recommends rather than announcing a deprecation. FutureWarning is also shown by default, and since it fires once per notary that is one line per process, not one per notebook. Using the notary as a context manager is included in the nudge: it only shipped in 5.11, so there is little code to move and moving it now is cheaper than letting more accumulate.
Add versionadded/versionchanged directives to the docstrings and to docs/api.rst: - 5.11 for what shipped in that release: `close()`, `__enter__`/`__exit__` and the `NotebookNotaryContext` protocol (all from jupyter#436). - 5.12 for what is new here: `NotarySession` and `open_session()`, plus the behaviour changes to `close()`, `store`, the context manager, and the three direct signing methods that now warn. 5.12 is the assumption that the next release is a minor one; if it ships as something else these need updating.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Notably, I think the main error of notary is to open the store at creation and not make it obvious it needs to be closed.
We can't break user too much, and give a progressive migration path.
Originally I wanted the notary context manager to return a new object (which I do here with Claude), returning a
Sessionobject which does open and close its store.The tough part:
#456 should be merged first as it's pure refactor to make it easier to review the code in depth here, right now I'm more trying to think at high level API and concepts/guaranties we want