-
Notifications
You must be signed in to change notification settings - Fork 892
Uv dev install #7372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uv dev install #7372
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
for more information, see https://pre-commit.ci
|
@dmayilyan thanks for the contribution! we will take a look after the release today |
| docs_url = "https://pip.pypa.io/" | ||
|
|
||
| def install_command(self, package: str, *, upgrade: bool) -> list[str]: | ||
| @override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately override was only introduced in 3.12, so this likely will break earlier versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I will try to avoid override (also typing_extensions based) altogether
| const response = await addPackage({ | ||
| package: packageSpec, | ||
| dev: true, | ||
| } as any); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you run make fe-codegen, it will update the typings so you can remove the any
| const response = await removePackage({ | ||
| package: packageName, | ||
| dev: isDev, | ||
| } as any); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you run make fe-codegen, it will update the typings so you can remove the any
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for UV dev dependencies by introducing a dev parameter to package installation and uninstallation operations. When installing packages from the "Optional Dependencies" UI menu, they are now installed as dev dependencies in UV projects using the --dev flag. Similarly, when uninstalling packages tagged with the "dev" group, the --dev flag is passed to the uninstall command.
Key Changes
- Added
devparameter toAddPackageRequestandRemovePackageRequestmodels in the backend - Updated package manager interface and implementations to accept and handle the
devparameter - Modified frontend to pass
dev=truewhen installing from Optional Dependencies and when uninstalling dev-tagged packages - Added comprehensive test coverage for dev dependency install/uninstall operations
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| marimo/_server/models/packages.py | Added dev field to AddPackageRequest and RemovePackageRequest models |
| marimo/_server/api/endpoints/packages.py | Updated endpoints to extract and pass dev parameter to package manager |
| marimo/_runtime/packages/package_manager.py | Added dev parameter to abstract package manager interface |
| marimo/_runtime/packages/pypi_package_manager.py | Implemented --dev flag support for UV projects; added parameter to all package manager implementations |
| marimo/_runtime/packages/conda_package_manager.py | Added dev parameter to Pixi package manager |
| frontend/src/components/app-config/optional-features.tsx | Pass dev=true when installing packages from Optional Dependencies |
| frontend/src/components/editor/chrome/panels/packages-panel.tsx | Check for dev group tag and pass dev parameter when uninstalling |
| tests/_server/api/endpoints/test_packages.py | Updated existing tests with dev=False parameter; added tests for dev dependency operations |
| tests/_runtime/packages/test_pypi_package_manager.py | Updated all install tests to include dev parameter; added test for UV dev dependency installation |
| tests/_runtime/packages/test_package_managers.py | Updated mock package manager tests to handle dev parameter |
Comments suppressed due to low confidence (3)
marimo/_runtime/packages/pypi_package_manager.py:87
- The
devparameter is accepted but not used in theuninstallmethod. For PIP, there's no native support for dev dependencies, so this parameter is effectively ignored. This should be documented or the method should warn/error ifdev=Trueis passed.
@override
async def uninstall(self, package: str, dev: bool = False) -> bool:
LOGGER.info(f"Uninstalling {package} with pip")
return self.run(
[
"pip",
"--python",
PY_EXE,
"uninstall",
"-y",
*split_packages(package),
],
log_callback=None,
)
marimo/_runtime/packages/pypi_package_manager.py:145
- The
devparameter is accepted but not used in theuninstallmethod. For Micropip (Pyodide), there's no native support for dev dependencies. The parameter should be documented as ignored or raise an error if set toTrue.
@override
async def uninstall(self, package: str, dev: bool = False) -> bool:
assert is_pyodide()
import micropip # type: ignore
try:
micropip.uninstall(package)
return True
except ValueError:
return False
marimo/_runtime/packages/pypi_package_manager.py:651
- The
devparameter is accepted but not used in theuninstallmethod. For Poetry, there's no native support for dev dependencies in the command construction. The parameter should either be documented as ignored or used to add the--group devflag if that's the intended behavior.
@override
async def uninstall(self, package: str, dev: bool = False) -> bool:
return self.run(
["poetry", "remove", "--no-interaction", *split_packages(package)],
log_callback=None,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| del dev | ||
| del upgrade | ||
| if log_callback: | ||
| log_callback(f"Installing {package}...\n") |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dev parameter is accepted but immediately deleted without being used. While this is a test mock, it should ideally validate or at least acknowledge the parameter to ensure the interface contract is being tested properly.
| del dev | |
| del upgrade | |
| if log_callback: | |
| log_callback(f"Installing {package}...\n") | |
| # Acknowledge dev and upgrade parameters in the log message | |
| if log_callback: | |
| log_callback( | |
| f"Installing {package}{' (dev)' if dev else ''}{' (upgrade)' if upgrade else ''}...\n" | |
| ) |
| return [ | ||
| "rye", | ||
| *(["sync", "--update"] if upgrade else ["add"]), | ||
| *split_packages(package), | ||
| ] | ||
|
|
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dev parameter is accepted but not used in the install_command method. For Rye, there's no native support for dev dependencies in the command construction. The parameter should either be documented as ignored or used to add appropriate Rye flags if supported.
| return [ | |
| "rye", | |
| *(["sync", "--update"] if upgrade else ["add"]), | |
| *split_packages(package), | |
| ] | |
| """ | |
| Construct the rye install command. | |
| Note: The `dev` parameter is accepted for interface compatibility, | |
| but is ignored because Rye does not support installing dev dependencies | |
| via a CLI flag. | |
| """ | |
| return [ | |
| "rye", | |
| *(["sync", "--update"] if upgrade else ["add"]), | |
| *split_packages(package), | |
| ] |
|
|
||
| async def uninstall(self, package: str) -> bool: | ||
| @override | ||
| async def uninstall(self, package: str, dev: bool = False) -> bool: |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dev parameter is accepted but not used in the uninstall method. For Rye, there's no native support for dev dependencies in the command construction. The parameter should either be documented as ignored or used to add appropriate Rye flags if supported.
| async def uninstall(self, package: str, dev: bool = False) -> bool: | |
| async def uninstall(self, package: str, dev: bool = False) -> bool: | |
| """ | |
| Uninstall a package using Rye. | |
| The `dev` parameter is accepted for interface compatibility but is ignored, | |
| as Rye does not support uninstalling dev dependencies separately. | |
| """ |
| @override | ||
| def install_command( | ||
| self, package: str, *, upgrade: bool, dev: bool | ||
| ) -> list[str]: | ||
| return [ | ||
| "poetry", | ||
| "update" if upgrade else "add", | ||
| "--no-interaction", | ||
| *split_packages(package), | ||
| ] |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dev parameter is accepted but not used in the install_command method. For Poetry, there's no native support for dev dependencies in the command construction. The parameter should either be documented as ignored or used to add the --group dev flag if that's the intended behavior.
| return [ | ||
| "pixi", | ||
| "upgrade" if upgrade else "add", | ||
| *split_packages(package), | ||
| ] | ||
|
|
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dev parameter is accepted but not used in the install_command method. For Pixi, there's no native support for dev dependencies in the command construction. The parameter should either be documented as ignored or used to add appropriate Pixi flags if supported.
| return [ | |
| "pixi", | |
| "upgrade" if upgrade else "add", | |
| *split_packages(package), | |
| ] | |
| """ | |
| Construct the Pixi install command. | |
| The `dev` parameter is accepted for interface compatibility, | |
| but is ignored because Pixi does not support installing dev dependencies | |
| via the command line. | |
| """ | |
| return [ | |
| "pixi", | |
| "upgrade" if upgrade else "add", | |
| *split_packages(package), | |
| ] |
| const response = await addPackage({ | ||
| package: packageSpec, | ||
| dev: true, | ||
| } as any); |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using as any bypasses TypeScript's type checking. The AddPackageRequest type should be updated to include the dev field, or the OpenAPI schema should be regenerated from the updated backend models to properly type this request.
| } as any); | |
| }); |
|
|
||
| async def uninstall(self, package: str) -> bool: | ||
| @override | ||
| async def uninstall(self, package: str, dev: bool = False) -> bool: |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dev parameter is accepted but not used in the uninstall method. For Pixi, there's no native support for dev dependencies in the command construction. The parameter should either be documented as ignored or used to add appropriate Pixi flags if supported.
| async def uninstall(self, package: str, dev: bool = False) -> bool: | |
| async def uninstall(self, package: str, dev: bool = False) -> bool: | |
| """ | |
| Uninstall a package using Pixi. | |
| The `dev` parameter is accepted for compatibility but ignored, | |
| as Pixi does not support uninstalling dev dependencies via a flag. | |
| """ |
| const response = await removePackage({ | ||
| package: packageName, | ||
| dev: isDev, | ||
| } as any); |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using as any bypasses TypeScript's type checking. The RemovePackageRequest type should be updated to include the dev field, or the OpenAPI schema should be regenerated from the updated backend models to properly type this request.
| } as any); | |
| }); |
|
hey @dmayilyan, i just approved/ran the test. there are a few lint and test errors, although I think the bottom two might be broken on main currently. Ive pasted the commands and output. let me know if you need help
|
|
Indeed when I did a pull and rerun the tests, I see the errors you sent. I guess I missed a minor release in between. |
|
@mscolnick Fixed the failing tests |
|
Hello @mscolnick As I see, failing tests are not related to my code. Am I missing something? When ran locally these tests where failing as they do here. Is that an expected behavior? Thanks |
|
Hi sorry @dmayilyan the tests look good now. We have just been busy lately but @manzt will review this on Monday |
|
Thanks @mscolnick |
manzt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work! I have some general questions about expected behavior with defaults for Optional Features and explicit {dev: true} always.
Second, I tried adding pandas --dev in the package sidebar in sandbox mode (marimo edit --sandbox foo.py) and got:
Failed to add package
Failed to add pandas --dev. See terminal for log errors.Logs:
# error: unexpected argument '--dev' found
#
# tip: a similar argument exists: '--deps'
#
# Usage: uv pip install --compile-bytecode <PACKAGE|--requirements <REQUIREMENTS>|--editable <EDITABLE>|--group <GROUP>>
#
# For more information, try '--help'.It's expected behavior that you can't use --dev with --script in uv add, but I'm surprised we are trying to invoke pip install ever with --dev. We should probably remove that code path.
| }) | ||
| .join(" "); | ||
| const response = await addPackage({ package: packageSpec }); | ||
| const response = await addPackage({ package: packageSpec, dev: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking through the implications of {dev: true} here...
I think it's sensible for most optional features since they're editor-related (formatting, AI, LSP, etc.). My instinct is that we want to avoid writing metadata that would break:
uv run --no-dev my_notebook.pyThe use case being that I have many of my "editor" features as optional deps, but then I can run my notebooks as scripts more efficiently.
The main case I can think of where this could be problematic is SQL cells - duckdb is a hard runtime dependency if the notebook actually uses SQL cells. The others should be safe as dev deps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree about duckdb. Besides, Altair can be an issue, as it may be someone's plotting tool in addition to being marimo's dev dependency.
There was an idea to have check boxes in the Optional Dependencies menu so that they can be selectively set to be installed in dev. That, on the other hand raises a new layer of conditions a package manager supporting dev or not and check marks being in the UI altogether/being inactive etc. I am not sure whether such a complex approach we would give us more value rather than visual/logical clutter to the product. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about the sandbox issue you mentioned and In my perception that's how it should behave. In my view where there is an expectation of package name in the sidebar --dev should not be parsed. It's a bit stylistic choice, so it is hard to argue :). I would go with a checkbox (or dropdown list) near the Add button:

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about the sandbox issue you mentioned and In my perception that's how it should behave.
Looking back, I agree with this. I thought we were passing down --dev somewhere explicitly, but we just forward the args there down to the command line. I wouldn't worry about the dev checkbox because it's the same issue we'd have to resolve with package managers that do not support a notion of "dev" dependencies.
frontend/src/components/editor/chrome/panels/packages-panel.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Trevor Manz <trevor.j.manz@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to go! The unit test failing I believe is unrelated (#7563).
It seems the api.yaml was manually edited. Could you regenerate it by running: make fe-codegen
This will regenerate the OpenAPI spec and TypeScript types from the Python models.
The schema check failure is actually a pre-existing CI issue - oasdiff can't parse the api.yaml on main:
Error: failed to load base spec from "main/packages/openapi/api.yaml"
But the changes here are backwards-compatible and should be fine to merge.
📝 Summary
For UV projects redirect dev dependencies from Settings -> Optional Dependencies UI menu to be installed in dev dependency group. As mentioned menu is targeting development tools proposed change fits the logic of isolation.
🔍 Description of Changes
--devflag when install request comes from Optional Dependencies.--devflag when uninstalling a package which is in dev dependency group, by passing tag content📋 Checklist