Add modal shell im-* to shell into an image id
#3719
Conversation
|
There might be a conflict here with some broader refactoring that @ehdr is doing, let's check in with him to avoid any issues before merging.
I think this design is subject to change by the way. |
|
Thanks for flagging, but no need to block on that! It's not urgent and not sure when(/if) I'll finish it, so go ahead! |
| volumes = {} if volume is None else {f"/mnt/{vol}": Volume.from_name(vol) for vol in volume} | ||
| secrets = [] if secret is None else [Secret.from_name(s) for s in secret] | ||
| start_shell = partial( | ||
| interactive_shell, | ||
| image=modal_image, | ||
| cpu=cpu, | ||
| memory=memory, | ||
| gpu=gpu, | ||
| cloud=cloud, | ||
| volumes=volumes, | ||
| secrets=secrets, | ||
| region=region.split(",") if region else [], | ||
| pty=pty, |
There was a problem hiding this comment.
The thing that bothers me about this (and where we got stuck in @ehdr's previous effort, motivating his attempt at a refactor) is that it makes it harder to explain the modal shell parameterization. Currently we have a (relatively) understandable heuristic that you pass REF or you pass a specification in the flags. This adds complexity because you need to say "If not passing REF, or if REF is an image ID".
OTOH it's not actively in conflict like it is with a task / sandbox ID (Function reference is more ambiguous since you could define "override" semantics for the additional flags, we just don't currently do that) and I can see how it might be useful to drop into a specific Image with a GPU etc...
There was a problem hiding this comment.
I think I'd be 👍 if we can plan to implement "override" semantics in the function reference case, so that the additional flags work where they "could" and it's not too hard to explain why you can't, e.g., start a shell into an existing container and add a GPU.
| ref: Optional[str] = typer.Argument( | ||
| default=None, | ||
| help=( | ||
| "ID of running container or Sandbox, or path to a Python file containing an App." | ||
| " Can also include a Function specifier, like `module.py::func`, if the file defines multiple Functions." | ||
| ), | ||
| ), |
There was a problem hiding this comment.
We need some doc improvements too right?
Describe your changes
Every so often, when debugging an image, I find myself creating a function just so I can shell into it. With this PR, you can now run
modal shell im-*.Aside: One downside is that Image IDs do not contain your mounts (by design). From a DevX prospective, it'll be nice to also have a
modal shell my_mod.py::my_image, where it builds the image and shells into it with all your mounts. It'll make it faster to iterate on an image definition.Checklists
Compatibility checklist
Check these boxes or delete any item (or this section) if not relevant for this PR.
Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.
Release checklist
If you intend for this commit to trigger a full release to PyPI, please ensure that the following steps have been taken:
modal_version/__init__.py) has been updated with the next logical versionChangelog
modal shellnow supports Image IDs for debugging image builds or exploring the files on your image.