-
Notifications
You must be signed in to change notification settings - Fork 177
Auto activate uv venv with pip installed #2666
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
from dstack._internal.core.errors import DockerRegistryError, ServerClientError | ||
from dstack._internal.core.models.common import RegistryAuth | ||
from dstack._internal.core.models.configurations import ( | ||
DEFAULT_REPO_DIR, | ||
PortMapping, | ||
PythonVersion, | ||
RunConfigurationType, | ||
|
@@ -149,7 +150,8 @@ async def _commands(self) -> List[str]: | |
commands = self.run_spec.configuration.commands | ||
elif shell_commands := self._shell_commands(): | ||
entrypoint = [self._shell(), "-i", "-c"] | ||
commands = [_join_shell_commands(shell_commands)] | ||
dstack_image_commands = self._dstack_image_commands() | ||
commands = [_join_shell_commands(dstack_image_commands + shell_commands)] | ||
else: # custom docker image without commands | ||
image_config = await self._get_image_config() | ||
entrypoint = image_config.entrypoint or [] | ||
|
@@ -164,6 +166,18 @@ async def _commands(self) -> List[str]: | |
|
||
return result | ||
|
||
def _dstack_image_commands(self) -> List[str]: | ||
if ( | ||
self.run_spec.configuration.image is not None | ||
or self.run_spec.configuration.entrypoint is not None | ||
): | ||
return [] | ||
return [ | ||
f"uv venv --prompt workflow --seed {DEFAULT_REPO_DIR}/.venv > /dev/null 2>&1", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered creating a global uv venv outside the repo. The problem with that is that having an active venv outside the repo would trigger uv warnings about the active repo not being used cause it defaults to As regards to .venv in git, it will never appear in git since it's always excluded by .venv/.gitignore. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Users may still be confused by a new directory in their repository, especially if they don't use uv or Python. But I guess we can live with that if there are no good alternatives There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe, let's see how it works |
||
f"echo 'source {DEFAULT_REPO_DIR}/.venv/bin/activate' >> ~/.bashrc", | ||
f"source {DEFAULT_REPO_DIR}/.venv/bin/activate", | ||
] | ||
|
||
def _app_specs(self) -> List[AppSpec]: | ||
specs = [] | ||
for i, pm in enumerate(filter_reserved_ports(self._ports())): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
__version__ = "0.0.0" | ||
__is_release__ = False | ||
base_image = "0.7" | ||
base_image = "0.8" |
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.
(nit) This means pip won't work if our base image is explicitly specified in
image
. I've seen some users do it, possibly because we advertise it in the docs (1, 2, and more).Maybe we could set up the venv in the Dockerfile, so that our images already come with a working pip?
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 current idea is to have .venv in the repo dir so that uv does not issue warnings about the wrong venv. And I don't want to hardcode the repo path in the image since we had plans to make it configurable.