fix: tag install location on ModuleNotFound to distinguish installation on server vs kernel#8619
Merged
fix: tag install location on ModuleNotFound to distinguish installation on server vs kernel#8619
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
mscolnick
reviewed
Mar 11, 2026
marimo/_runtime/packages/utils.py
Outdated
| ) | ||
|
|
||
|
|
||
| def can_server_auto_install() -> bool: |
Contributor
There was a problem hiding this comment.
maybe just better to prompt the user? its not that big of a deal, and they can change the version (and maybe index-url in future)
mscolnick
reviewed
Mar 11, 2026
mscolnick
reviewed
Mar 11, 2026
mscolnick
reviewed
Mar 11, 2026
| name: ClassVar[str] = "missing-package-alert" | ||
| packages: list[str] | ||
| isolated: bool | ||
| source: Literal["kernel", "server"] = "kernel" |
Contributor
There was a problem hiding this comment.
can we reduce the places where we have this default? ideally it all gets funneled through and don't need defaults
Contributor
There was a problem hiding this comment.
cc @manzt does this need to be optional to avoid breaking marimo-LSP
Collaborator
There was a problem hiding this comment.
I think this should be ok because we just decode JSON to dict to forward to LSP:
mscolnick
reviewed
Mar 11, 2026
mscolnick
reviewed
Mar 11, 2026
mscolnick
reviewed
Mar 11, 2026
mscolnick
reviewed
Mar 11, 2026
| session_id = app_state.get_current_session_id() | ||
| session = app_state.get_current_session() | ||
| if session_id is not None and session is not None: | ||
| send_message_to_consumer( |
Contributor
There was a problem hiding this comment.
this logic (although maybe only 20 lines) seems deduped. could be worth DRYing
dmadisetti
force-pushed
the
dm/tag-request
branch
from
e528bd5 to
01ab1c8
Compare
dmadisetti
force-pushed
the
dm/tag-request
branch
from
069fb91 to
eda89c7
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an explicit “installation target” to missing-package alerts and install requests so Marimo can distinguish packages needed by the server vs the kernel (notably for --sandbox / auto-export + formatting flows).
Changes:
- Introduces
source: "kernel" | "server"on install commands and missing-package notifications; propagates through backend models, OpenAPI, and frontend alert/install UI. - Routes certain server-owned dependency checks (e.g.
nbformatfor IPYNB auto-export,rufffor formatting, export/AI deps) to trigger server installs/alerts instead of kernel installs. - Adjusts IPC sandbox kernel env to avoid inheriting outer
uvproject environment variables.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/_server/test_errors.py | Updates ManyModulesNotFoundError construction to include source. |
| tests/_server/api/endpoints/test_editing.py | Adds endpoint test coverage for source="server" install requests. |
| tests/_runtime/test_manage_script_metadata.py | Updates ManyModulesNotFoundError construction to include source. |
| tests/_dependencies/test_dependencies.py | Updates DependencyManager.require_many tests for new source parameter. |
| packages/openapi/src/api.ts | Updates generated TS OpenAPI types to include source. |
| packages/openapi/api.yaml | Updates OpenAPI schema/docs to include source and clarify isolated. |
| marimo/_sql/sql.py | Tags SQL dependency requirements as source="kernel". |
| marimo/_session/managers/ipc.py | Overrides env vars for sandbox kernel subprocess environment. |
| marimo/_server/models/models.py | Propagates source from request to InstallPackagesCommand. |
| marimo/_server/export/exporter.py | Marks export dependencies as server-side (source="server"). |
| marimo/_server/errors.py | Includes source in missing-package notifications; adjusts isolated based on source. |
| marimo/_server/api/utils.py | Adds helper to notify missing server packages. |
| marimo/_server/api/endpoints/export.py | Sends a server-side missing-package alert for nbformat before scheduling background export. |
| marimo/_server/api/endpoints/editing.py | Sends server-side missing-package alert for formatter deps; adds server-side install path to install endpoint. |
| marimo/_server/ai/providers.py | Marks AI provider deps as source="server". |
| marimo/_runtime/commands.py | Adds source field to InstallPackagesCommand. |
| marimo/_messaging/notification.py | Adds source field to MissingPackageAlertNotification and updates docs. |
| marimo/_dependencies/errors.py | Extends ManyModulesNotFoundError to carry source. |
| marimo/_dependencies/dependencies.py | Requires callers to provide source when raising ManyModulesNotFoundError. |
| marimo/_cli/export/commands.py | Marks CLI export deps as source="server". |
| frontend/src/core/alerts/state.ts | Adds source to missing package alert state. |
| frontend/src/components/editor/package-alert.tsx | Passes source through to install request from the missing-package alert. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+15
to
23
| def __init__( | ||
| self, | ||
| package_names: list[str], | ||
| msg: str, | ||
| source: Literal["kernel", "server"], | ||
| ) -> None: | ||
| self.package_names = package_names | ||
| self.source = source | ||
| super().__init__(msg) |
Comment on lines
+120
to
+131
| response = client.post( | ||
| "/api/kernel/install_missing_packages", | ||
| headers=HEADERS, | ||
| json={ | ||
| "manager": "pip", | ||
| "versions": {"nbformat": ""}, | ||
| "source": "server", | ||
| }, | ||
| ) | ||
| assert response.status_code == 200, response.text | ||
| assert response.headers["content-type"] == "application/json" | ||
| assert "success" in response.json() |
Comment on lines
+254
to
+267
| body = await parse_request(request, cls=InstallPackagesRequest) | ||
| cmd = body.as_command() | ||
|
|
||
| if cmd.source == "server": | ||
| # Install into the server's own Python environment (sys.executable). | ||
| # Used when the server itself needs a package (e.g. nbformat for | ||
| # IPYNB auto-export when running with --sandbox). | ||
| pkg_manager = create_package_manager(cmd.manager, python_exe=None) | ||
| if not pkg_manager.is_manager_installed(): | ||
| pkg_manager.alert_not_installed() | ||
| else: | ||
| for pkg, version in cmd.versions.items(): | ||
| await pkg_manager.install(pkg, version=version or None) | ||
| return SuccessResponse() |
Comment on lines
+263
to
+266
| pkg_manager.alert_not_installed() | ||
| else: | ||
| for pkg, version in cmd.versions.items(): | ||
| await pkg_manager.install(pkg, version=version or None) |
| def require_many( | ||
| why: str, | ||
| *dependencies: Dependency, | ||
| source: Literal["kernel", "server"], |
mscolnick
reviewed
Mar 15, 2026
| # Install into the server's own Python environment (sys.executable). | ||
| # Used when the server itself needs a package (e.g. nbformat for | ||
| # IPYNB auto-export when running with --sandbox). | ||
| pkg_manager = create_package_manager(cmd.manager, python_exe=None) |
Contributor
There was a problem hiding this comment.
do we want to use python_exe=sys.executable? is it safe long-term to rely on the default?
mscolnick
reviewed
Mar 15, 2026
| ) | ||
| packages = [response.name] | ||
| source = "kernel" | ||
| isolated = True if source == "server" else is_python_isolated() |
Contributor
There was a problem hiding this comment.
why is it isolated if source == "server"?
mscolnick
reviewed
Mar 15, 2026
marimo/_session/managers/ipc.py
Outdated
| # venv as its environment, not the outer uv project. | ||
| env["VIRTUAL_ENV"] = str(Path(venv_python).parent.parent) | ||
| env.pop("UV_PROJECT_ENVIRONMENT", None) | ||
|
|
Contributor
There was a problem hiding this comment.
are there tests for this? maybe we can have a function construct_env which takes in the previous env and compares the expected output
mscolnick
reviewed
Mar 15, 2026
Contributor
mscolnick
left a comment
mscolnick
left a comment
There was a problem hiding this comment.
it would be good to make some of this code more testable.
mscolnick
reviewed
Mar 15, 2026
|
|
||
|
|
||
| @with_session(SESSION_ID) | ||
| def test_install_missing_packages_server_source(client: TestClient) -> None: |
Contributor
There was a problem hiding this comment.
this does not actually test what it says?
mscolnick
previously approved these changes
Mar 16, 2026
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.20.5-dev89 |
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.
📝 Summary
Tag installation requests with either server-side or notebook level installation for
--sandboxmode.Tested in:
We might consider a more visual indication of installation location?