Skip to content

Conversation

@m1so
Copy link
Member

@m1so m1so commented Oct 7, 2025

part of OSS-135

related to https://github.com/deepnote/deepnote-toolkit/pull/218

Summary by CodeRabbit

  • Chores
    • Enforced pip constraints during server startup to ensure installed packages follow defined versions.
    • Updated the Deepnote toolkit package reference to a newer post-release to align installer sources.
  • Bug Fixes
    • Reduced dependency conflicts during kernel startup, improving session reliability and reducing installation-related errors.

@m1so m1so requested a review from mfranczel October 7, 2025 11:00
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 7, 2025

📝 Walkthrough

Walkthrough

In DeepnoteServerStarter.startServerImpl (src/kernels/deepnote/deepnoteServerStarter.node.ts), the process environment prepared for server startup now includes DEEPNOTE_ENFORCE_PIP_CONSTRAINTS='true' — set after VIRTUAL_ENV and before removing PYTHONHOME. Separately, the Deepnote toolkit wheel URL constant in src/kernels/deepnote/types.ts was updated to use DEEPNOTE_TOOLKIT_VERSION='0.2.30.post23' (wheel URL now derived from that template). No control flow, error handling, or public/exported signatures were changed.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Starter as DeepnoteServerStarter
    participant Env as Process Env
    participant Spawn as Child Process (Jupyter server)
    note over Starter,Env #f8fbff: startServerImpl prepares environment

    Starter->>Env: set VIRTUAL_ENV
    Starter->>Env: set DEEPNOTE_ENFORCE_PIP_CONSTRAINTS = "true"
    Starter->>Env: remove PYTHONHOME
    note right of Env #eef8f1: Env includes DEEPNOTE_ENFORCE_PIP_CONSTRAINTS

    Starter->>Spawn: spawn(server, env=Env)
    note right of Spawn #fff8e1: Jupyter server inherits env
    Spawn->>Spawn: on startup, pip install flows may use\nPIP constraints (if implemented)

    note over Starter,Spawn #f7f7f7: types.ts: DEEPNOTE_TOOLKIT_VERSION = 0.2.30.post23\nDEEPNOTE_TOOLKIT_WHEEL_URL derived from version
Loading

Possibly related PRs

  • deepnote/deepnote-toolkit#218: Implements behavior that reads DEEPNOTE_ENFORCE_PIP_CONSTRAINTS to inject PIP_CONSTRAINTS into the server environment — directly related to the new env var being set.

Pre-merge checks

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Linked Issues Check ⚠️ Warning The PR adds an internal flag and bumps the toolkit version but does not set the PIP_CONSTRAINT environment variable to point at a Deepnote-hosted constraints file per Python version as specified in OSS-135, so it fails to enforce actual pip version constraints. Implement logic to set the PIP_CONSTRAINT environment variable to the appropriate Deepnote constraints URL based on Python version and ensure pip installs respect those constraints to satisfy OSS-135.
Out of Scope Changes Check ⚠️ Warning The bump of DEEPNOTE_TOOLKIT_VERSION and the wheel URL update are unrelated to enforcing pip constraints and fall outside the objectives of OSS-135, which focuses solely on setting and using a constraints file. Remove or isolate the version bump and wheel URL template change into a separate PR and keep this pull request focused exclusively on implementing pip constraint enforcement.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly captures the primary purpose of the changeset, which is to prevent users from breaking the toolkit’s dependencies via pip installs, and it avoids unnecessary detail or noise.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3efe8fc and 294504c.

📒 Files selected for processing (1)
  • src/kernels/deepnote/types.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/!(*.node|*.web).ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place shared cross-platform logic in common .ts files (not suffixed with .node or .web)

Files:

  • src/kernels/deepnote/types.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation with CancellationToken

**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility, then alphabetically
Separate third-party imports from local file imports

Files:

  • src/kernels/deepnote/types.ts
src/kernels/**/*.ts

📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)

src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations

Files:

  • src/kernels/deepnote/types.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: hc2p
PR: deepnote/environments#62
File: gpu/Dockerfile:0-0
Timestamp: 2025-09-25T13:16:13.796Z
Learning: In Deepnote's Docker environment setup, packages installed in the underlying Docker image should be in system site packages, not in virtual environments. Server/kernels are launched in a virtual environment created by the installer, which creates a pth file where system site packages are appended first, giving them higher precedence and making them first in the search path. This is expected behavior for the deepnote-toolkit setup.
🔇 Additional comments (1)
src/kernels/deepnote/types.ts (1)

144-145: Approve template-based wheel URL refactor.
Wheel file at the generated URL returns HTTP 200. Confirm the staging artifactory URL is intended for this environment.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 52a37ce and 3efe8fc.

📒 Files selected for processing (1)
  • src/kernels/deepnote/types.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/!(*.node|*.web).ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place shared cross-platform logic in common .ts files (not suffixed with .node or .web)

Files:

  • src/kernels/deepnote/types.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation with CancellationToken

**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility, then alphabetically
Separate third-party imports from local file imports

Files:

  • src/kernels/deepnote/types.ts
src/kernels/**/*.ts

📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)

src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations

Files:

  • src/kernels/deepnote/types.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: hc2p
PR: deepnote/environments#62
File: gpu/Dockerfile:0-0
Timestamp: 2025-09-25T13:16:13.796Z
Learning: In Deepnote's Docker environment setup, packages installed in the underlying Docker image should be in system site packages, not in virtual environments. Server/kernels are launched in a virtual environment created by the installer, which creates a pth file where system site packages are appended first, giving them higher precedence and making them first in the search path. This is expected behavior for the deepnote-toolkit setup.
🔇 Additional comments (1)
src/kernels/deepnote/types.ts (1)

145-145: Wheel URL verified. URL returns HTTP 200 OK.

@m1so m1so merged commit 8d67855 into main Oct 7, 2025
4 checks passed
@m1so m1so deleted the mb/prevent-user-from-breaking-toolkit-dependencies branch October 7, 2025 13:44
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.

3 participants