perf(e2e): adopt a pre-baked toolkit venv instead of provisioning per suite - #470
Draft
tkislan wants to merge 1 commit into
Draft
perf(e2e): adopt a pre-baked toolkit venv instead of provisioning per suite#470tkislan wants to merge 1 commit into
tkislan wants to merge 1 commit into
Conversation
… suite
Profiling a full E2E run (CI 32039112714) showed 88% of the 15.8 min mocha
phase is before() hooks, not assertions: 837s of setup against ~111s of test
bodies. 402s of that setup was provisioning Python venvs, because four suites
each created a differently-named Deepnote environment and every distinct name
means another venv plus a full deepnote-toolkit pip install.
Environments are global (globalState + globalStorageUri/deepnote-venvs), so a
shared name is safe across suites regardless of which workspace is open, and
the sharing pattern already existed — two suites carried a "shared env so CI
provisions one venv" comment. initNotebookRunner (197s, the single most
expensive suite) and integrationsEnvFileInjection had each drifted onto their
own name.
Rather than keep fixing that by hand, the name now lives in one constant and
the venv itself is baked once:
- SHARED_ENV_NAME in test/e2e/helpers/constants.ts, imported by all five
sharing suites. 'E2E Delete Env' stays a literal — that suite deletes what it
creates, so it must not share.
- build/e2e/prepareE2eVenv.js bakes .venv-e2e with the exact set
deepnoteToolkitInstaller installs, reading DEEPNOTE_TOOLKIT_VERSION from
source so it cannot drift. It is idempotent and self-healing: a venv that
cannot import deepnote_toolkit is discarded and rebuilt, which also covers a
restored cache whose base interpreter moved.
- createEnvironment now selects the interpreter deterministically instead of
selectQuickPick(0). getVenvPathIfInVenv makes the extension adopt any
interpreter already inside a venv, and ensureVenvAndToolkit returns early once
the toolkit imports, so adopting the baked venv skips creation and pip
entirely. A missing venv warns loudly and falls back, keeping the run slow
rather than red.
- The deletion suite opts out via createEnvironment(name, { useManagedVenv:
true }). deleteEnvironment only removes the venv directory for managed
environments, so adopting the baked venv there would silently stop exercising
that teardown.
- The interpreter path is only known at run time, so the script also emits
test/e2e/settings.generated.json (base settings + python.venvPath +
python.defaultInterpreterPath) and the extest scripts point at it.
Also fixes the pip cache, which had been frozen since its first save:
actions/cache only writes on a miss, and the key was a bare content hash that
kept hitting. Three of the four installed specs are unpinned and installed with
--upgrade, so newer wheels were downloaded every run and never written back.
The key now carries github.run_id with restore-keys falling back to the newest
matching entry, so each run starts warm and saves a refreshed copy.
Verified: tsc (compile-e2e) exits 0, the workflow YAML parses and its step order
is correct, the toolkit-version regex resolves 2.1.1 against the real source,
and settings generation produces valid JSON. NOT verified: no E2E run has
executed against this. The behavioural assumption that needs CI to confirm is
that the Python extension surfaces the baked venv in the interpreter quick pick
via python.venvPath.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MyZCw9GzL6Vq7S9MAVYwby
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #470 +/- ##
===========================
===========================
🚀 New features to boost your workflow:
|
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
Profiling a full E2E run (32039112714) showed the runtime is almost entirely setup, not testing:
Four suites each created a differently-named Deepnote environment, and every distinct name means another venv plus a full
deepnote-toolkitpip install:initNotebookRunnerE2E Init Envenvironment#1E2E Hello Envenvironment#2E2E Delete EnvintegrationsEnvFileInjectionE2E Integrations EnvhelloWorld/snapshotsE2E Hello EnvThe two that reused a name cost a fraction of the ones that didn't — the sharing pattern already worked, and two suites had simply drifted off it. Environments are global (
globalState+globalStorageUri/deepnote-venvs), so sharing a name is safe regardless of which workspace is open.Changes
One name, one place.
SHARED_ENV_NAMEintest/e2e/helpers/constants.ts, imported by all five sharing suites.'E2E Delete Env'stays a literal — that suite deletes what it creates, so it must not share. This is what stops the drift recurring.Bake the venv once.
build/e2e/prepareE2eVenv.jscreates.venv-e2ewith the exact setdeepnoteToolkitInstallerinstalls, readingDEEPNOTE_TOOLKIT_VERSIONfrom source so it cannot drift. Idempotent and self-healing — a venv that cannotimport deepnote_toolkitis discarded and rebuilt, which also covers a restored cache whose base interpreter moved.Adopt it instead of building one.
createEnvironmentnow selects the interpreter deterministically rather thanselectQuickPick(0).getVenvPathIfInVenvmakes the extension adopt any interpreter already inside a venv, andensureVenvAndToolkitreturns early once the toolkit imports — so adoption skips venv creation and pip entirely.The deletion suite keeps building its own. It opts out with
createEnvironment(name, { useManagedVenv: true }).deleteEnvironmentremoves the venv directory for managed environments only, so adopting the baked venv there would silently stop exercising that teardown. It stays self-contained and remains the one place real managed creation + teardown is covered.Generated settings. The interpreter must be named by absolute path, known only at run time, so the script also emits
test/e2e/settings.generated.json(base settings +python.venvPath+python.defaultInterpreterPath) and the extest scripts point at it. Gitignored.Also: the pip cache had been frozen since its first save
actions/cacheonly writes on a miss, and the key was a bare content hash that kept hitting:Three of the four installed specs are unpinned and installed with
--upgrade, so newer wheels were downloaded every run and never written back — the entry stayed at whatever existed when the key was last busted. The key now carriesgithub.run_id, withrestore-keysfalling back to the newest matching entry, so each run starts warm and saves a refreshed copy.Expected impact
Projected from measured per-suite costs, not observed — see below.
Verification
Done:
npm run compile-e2eexits 0; the workflow YAML parses and step order is correct; the toolkit-version regex resolves2.1.1against the real source; settings generation produces valid JSON.npm run lintcoverssrconly, so nothing here is linted by CI.Not done — no E2E run has executed against this. The behavioural assumption that needs CI to confirm is that the Python extension surfaces the baked venv in the interpreter quick pick via
python.venvPath. If it does not, the helper warns withno interpreter under .venv-e2e was offeredand falls back to the old behaviour, so the run stays green and merely slow. Check the E2E log for that warning before trusting the numbers.Follow-up, not in this PR
The remaining ~435s of setup is
openFolderViaDialogreloading the workbench once per suite, across 17 suites at 14–55s each. After this change that is the dominant cost, and it is a fixture restructure rather than a caching problem.🤖 Generated with Claude Code
https://claude.ai/code/session_01MyZCw9GzL6Vq7S9MAVYwby