Skip to content

feat(dataframe-storage): store a block's result automatically - #131

Draft
tkislan wants to merge 5 commits into
feat/artifacts-store-dataframefrom
feat/dataframe-storage
Draft

tkislan wants to merge 5 commits into
feat/artifacts-store-dataframefrom
feat/dataframe-storage

Conversation

@tkislan

@tkislan tkislan commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

@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_dataframe and 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_request metadata (deepnote.dataframeStorage = {name, blockId}). A post_run_cell hook then stores the cell's result under that name, as if the block ended with artifacts.store_dataframe(result, name), and records the block in the manifest as block_id. A later call to store_dataframe writes no block_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 in init_deepnote_runtime(), the metadata reader and the error reports.
  • deepnote_toolkit/artifacts/dataframe_storage_manifest.py: write_manifest takes an optional block_id.
  • DEEPNOTE_DATAFRAME_STORAGE_ENABLED switches the hook on. It does not affect store_dataframe.
  • No code is added to user cells.

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

  • Never raises, never prints into the cell. Warnings are suppressed. Failures are reported to the webapp once per kernel session per errno or exception type, as TOOLKIT_RUNTIME_ERROR with code: DATAFRAME_STORAGE_WRITE_FAILED (the type toolkit/errors sends to Sentry, grouped by code), with requests like 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.
  • Stop during a write. IPython would catch a re-raised KeyboardInterrupt, print "Error in callback" and keep the cell successful, so the hook sets result.error_in_exec and calls showtraceback(). ipykernel builds the reply from the result after the hook, so the reply carries KeyboardInterrupt and the executor stops its queue. No SIGINT handler is installed: a cell with top-level await runs post_run_cell under 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.
  • Untrusted inputs. name and blockId from request metadata must match [A-Za-z0-9_-]{1,128}, because the name becomes a directory.
  • Skipped: a read-only mount (its first write fails with EROFS before 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 by result.info.store_history, not execution_count: IPython 9 sets execution_count even when no history is stored, so the RFC's earlier "no execution_count" premise only holds on IPython 8.
  • Env var: enabled for 1, true, yes, on (case-insensitive), the same set as _to_bool in deepnote_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:

Not verified here

  • Python 3.10-3.12 and 3.14, and pyarrow 16.1. Only 3.13 / pyarrow 24.0 / pandas 2.2.3 / polars 1.39 ran locally.
  • A real s3fs mounter, and whether polars writing to a Python file object surfaces a failed upload (fallback: flush plus fsync).
  • A real kernel on the top-level-await path. IPython 9.12 (what the lock pins for Python 3.14) was run locally against the new tests, in a scratch install, not through a real kernel.
  • The project root copies set_notebook_path()'s precedence, which uses home_dir without a /work suffix. Confirm on a pod that it equals the project mount.
  • The DEEPNOTE_DATAFRAME_STORAGE_ENABLED value 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)
  • At the previous revision, the same tests under IPython 9.12.0, installed to a scratch dir (the lock's 3.14 pin): 229 passed
  • At the previous revision, broke the hook on purpose in 20 places (flag, name and block-id validation, helper-execution and failed-cell checks, SIGINT handling, report de-duplication, raise_for_status, warning suppression, read-only and EROFS handling, block_id, registration at startup); every break failed a test
  • black, isort, flake8 clean on the touched files; mypy deepnote_toolkit/ clean
  • Full tests/unit: 1510 passed, 9 skipped
  • CI across the Python / pyarrow matrix
  • Pod test: mount upload on close, project root

🤖 Generated with Claude Code

https://claude.ai/code/session_01B441SpUqRn7QTzcnWxjNa5

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

📦 Python package built successfully!

  • Version: 2.8.0.dev21+12d5154
  • Wheel: deepnote_toolkit-2.8.0.dev21+12d5154-py3-none-any.whl
  • Install:
    pip install "deepnote-toolkit @ https://deepnote-staging-runtime-artifactory.s3.amazonaws.com/deepnote-toolkit-packages/2.8.0.dev21%2B12d5154/deepnote_toolkit-2.8.0.dev21%2B12d5154-py3-none-any.whl"

@codecov

codecov Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.56%. Comparing base (3c36f45) to head (f437b05).
✅ All tests successful. No failed tests found.

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     
Flag Coverage Δ
combined 78.56% <100.00%> (+0.61%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@deepnote-bot

deepnote-bot commented Oct 6, 2026 •

Copy link
Copy Markdown

🚀 Review App Deployment Started

📝 Description 🌐 Link / Info
🌍 Review application ra-131
🔑 Sign-in URL Click to sign-in
📊 Application logs View logs
🔄 Actions Click to redeploy
🚀 ArgoCD deployment View deployment
⏰ Last deployed 2026-10-14 15:46:31 (UTC)
📜 Deployed commit d8c350ad6af10efeb02de3b1edffdee9cd7333c5
🛠️ Toolkit version 12d5154

@tkislan
tkislan force-pushed the feat/dataframe-storage branch from 91ffced to 3b63336 Compare October 8, 2026 12:55
@tkislan tkislan changed the title feat(dataframe-storage): store the full DataFrame of flagged blocks feat(dataframe-storage): store a block's result automatically Oct 8, 2026
@tkislan
tkislan changed the base branch from main to feat/artifacts-store-dataframe October 8, 2026 12:55
@tkislan
tkislan force-pushed the feat/dataframe-storage branch from 3b63336 to 0d1a11a Compare October 12, 2026 19:40
tkislan and others added 3 commits October 14, 2026 09:24
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
tkislan force-pushed the feat/dataframe-storage branch from 0d1a11a to b832280 Compare October 14, 2026 09:51
tkislan and others added 2 commits October 14, 2026 15:26
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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants