Repository navigation
Conversation
|
📦 Python package built successfully!
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/artifacts-store-dataframe #131 +/- ##
==================================================================
+ Coverage 77.95% 78.56% +0.61%
==================================================================
Files 118 118
Lines 6758 6845 +87
Branches 985 997 +12
==================================================================
+ Hits 5268 5378 +110
+ Misses 1181 1160 -21
+ Partials 309 307 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
🚀 Review App Deployment Started
|
tkislan
force-pushed
the
feat/dataframe-storage
branch
from
October 8, 2026 12:55
91ffced to
3b63336
Compare
tkislan
force-pushed
the
feat/dataframe-storage
branch
from
October 12, 2026 19:40
3b63336 to
0d1a11a
Compare
A `post_run_cell` hook stores the DataFrame a cell returns, under the name the
webapp passes in `execute_request` metadata (`deepnote.dataframeStorage` =
`{name, blockId}`), as `artifacts.store_dataframe` would, and records the block in
the manifest as `block_id`. `DEEPNOTE_DATAFRAME_STORAGE_ENABLED` switches the hook
on; it is registered as one more guarded step in `init_deepnote_runtime()`.
Unlike the function, the hook never raises and never prints into the cell: it
skips helper executions, failed cells, previews and PySpark frames and read-only
mounts, fails the execution when Stop raises KeyboardInterrupt during the write,
and reports failures to the webapp once per kernel session per errno or
exception type.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B441SpUqRn7QTzcnWxjNa5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FhzfrytCUojy8Vw1gbX3Bx
The pandas-on-Spark result test toggled ANSI mode by hand; the fixture does the same and restores it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FhzfrytCUojy8Vw1gbX3Bx
Drop docstring paragraphs and comments that restate the hook's code or the assertions beside them. Keep the external facts: what the webapp's toolkit/errors endpoint accepts, how IPython and ipykernel treat a post_run_cell callback, and why the report avoids report_error_to_webapp. Fix the ones that were wrong: the endpoint accepts TOOLKIT_IMPORT_ERROR too, report_error_to_webapp logs once rather than before every attempt, and the reply is built from the result after the hook, which is why setting `error_in_exec` reaches the executor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FhzfrytCUojy8Vw1gbX3Bx
tkislan
force-pushed
the
feat/dataframe-storage
branch
from
October 14, 2026 09:51
0d1a11a to
b832280
Compare
Bring in the store_dataframe changes this branch is stacked on. store_frame keeps this branch's block_id parameter and takes the base branch's Optional[str] return, with mkdir inside the try that maps a pre-commit EROFS to None. The report constants stay; the polars constants lose their comments, which moved to where they are used. The hook still calls the removed is_read_only; the next commit adapts it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FhzfrytCUojy8Vw1gbX3Bx
The base branch removed is_read_only: store_frame now returns None on a read-only mount, before converting, so the hook drops its probe. The hook's EROFS catch now only sees EROFS after the manifest step, which stays silent; a test covers that edge. Tests that patched DataFrame.to_feather or os.statvfs now target pyarrow's write_feather and the EROFS-raising read_only helper, which moves from test_artifacts into the shared test helpers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FhzfrytCUojy8Vw1gbX3Bx
This branch has not been deployed
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
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.
@coderabbitai ignore
Summary
Phase 2 of the "Full Dataframe storage" RFC (§3.6.4): storing a block's result automatically. Stacked on #135, which adds
artifacts.store_dataframeand the writer; this PR only adds the hook on top of it, so review that one first. The diff against it is the hook, its tests, one manifest field and the read-only test helper, now shared.When a Python or SQL block has a storage setting, the webapp puts the setting's frame name in the
execute_requestmetadata (deepnote.dataframeStorage={name, blockId}). Apost_run_cellhook then stores the cell's result under that name, as if the block ended withartifacts.store_dataframe(result, name), and records the block in the manifest asblock_id. A later call tostore_dataframewrites noblock_id, so a frame that code stored last belongs to no block.deepnote_toolkit/artifacts/dataframe_storage.py: the hook, registered as one more guarded step ininit_deepnote_runtime(), the metadata reader and the error reports.deepnote_toolkit/artifacts/dataframe_storage_manifest.py:write_manifesttakes an optionalblock_id.DEEPNOTE_DATAFRAME_STORAGE_ENABLEDswitches the hook on. It does not affectstore_dataframe.Out of scope (other repos): the block setting, webapp and executor metadata, the feature flag, the read endpoint, and the cleanup that deletes a block's frame when its setting is cleared.
Behaviour worth reviewing
TOOLKIT_RUNTIME_ERRORwithcode: DATAFRAME_STORAGE_WRITE_FAILED(the typetoolkit/errorssends to Sentry, grouped bycode), withrequestslike the other userpod-API callers. An error status from the webapp is logged to the file logger as well. The report carries the exception type, never its message, because pyarrow quotes the failing value.KeyboardInterrupt, print "Error in callback" and keep the cell successful, so the hook setsresult.error_in_execand callsshowtraceback(). ipykernel builds the reply from the result after the hook, so the reply carriesKeyboardInterruptand the executor stops its queue. No SIGINT handler is installed: a cell with top-level await runspost_run_cellunder a handler that only queues the interrupt, so Stop during that write is lost and the run goes on, as with any blocking code at the end of such a cell.nameandblockIdfrom request metadata must match[A-Za-z0-9_-]{1,128}, because the name becomes a directory.EROFSbefore anything is converted), SQL query previews, PySpark and pandas-on-Spark frames, non-DataFrame results, failed or interrupted cells, and helper executions. Helpers are recognised byresult.info.store_history, notexecution_count: IPython 9 setsexecution_counteven when no history is stored, so the RFC's earlier "noexecution_count" premise only holds on IPython 8.1,true,yes,on(case-insensitive), the same set as_to_boolindeepnote_core/config/loader.py.Changes since the previous revision of this PR
The RFC moved from per-block storage to named frames, and its first part (the function) moved to #135:
deepnote_dataframes/<name>/, not.deepnote/dataframes/<notebookId>/<blockId>/.{name, blockId}.enabledandnotebookIdare gone: the webapp adds the field only for executions that should store._sigint_raises) is gone, as is feat(artifacts): add store_dataframe and delete_dataframe #135's deferral of Stop during the manifest update.Not verified here
set_notebook_path()'s precedence, which useshome_dirwithout a/worksuffix. Confirm on a pod that it equals the project mount.DEEPNOTE_DATAFRAME_STORAGE_ENABLEDvalue comes from the sandbox manager, which appends a project's own variables after the system ones, so a project can override it (RFC §3.6.2). Not fixed here.Test plan
poetry run python -m pytest tests/unit/artifacts tests/unit/test_runtime_initialization.py: 231 passed (the 150 tests of feat(artifacts): add store_dataframe and delete_dataframe #135 and 81 for the hook)raise_for_status, warning suppression, read-only andEROFShandling,block_id, registration at startup); every break failed a testblack,isort,flake8clean on the touched files;mypy deepnote_toolkit/cleantests/unit: 1510 passed, 9 skipped🤖 Generated with Claude Code
https://claude.ai/code/session_01B441SpUqRn7QTzcnWxjNa5