-
Notifications
You must be signed in to change notification settings - Fork 65
Adds Image.build to eagerly build an image #3464
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
Conversation
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.
One thing I don't super love here is that nearly every other Image method is "recipe-oriented" (I think .imports()
is the only other exception?). OTOH I'm not certain that modal.build_image(image, app)
is that much better.
One thing I like about this is that we have a lot of internal uses of the pattern
app = modal.App()
@app.function(image=image)
def dummy():
...
with app.run():
...
As a way of building an Image on-demand and it will be nice to get rid of them!
modal/image.py
Outdated
``` | ||
""" | ||
if app.app_id is None: | ||
raise InvalidError("App has not been initialized yet. Run with `with app.run()`.") |
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.
Regarding "Run with with app.run()
, it would also be valid to use App.lookup
and if this is oriented towards Sandbox usecases that's likely the more common pattern. Not sure we need to call out a specific suggestion though, although it's arguable a flaw that we don't have a single clear remediation to point to :/.
OTOH if you change the suggestion to "Run App with with app.run()
" it will be a word-level palindrome and then I'd like It just for the luls 😁
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'll add App.lookup
in the error message as well.
For generic image building (independent of sandboxes) where you just want to build with and get an image id, I think an ephemeral app is a nicer (thus app.run()
).
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.
Yeah I agree that if you have some standalone build_modal_image.py
script that makes sense.
In the Sandbox usecase there might be a pattern more like
app = modal.App.lookup(...)
image = modal.Image.from_registry(...).build()
# Now we can start accepting requests or something else
sb = modal.Sandbox.create(app=app, image=image)
return obj | ||
|
||
async def build(self, app: "modal.app._App") -> "_Image": | ||
"""Eagerly build an image. |
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.
Couple suggestions for the documentation:
First, can we make it clear somehow that you don't normally need to do this when using Functions? i.e. we very much want to avoid people doing
app = modal.App()
image = modal.Image.debian_slim()
with app.run():
image.build(app)
@app.function()
def f():
...
Second, can we make it clear (again, somehow ... it's a little tricky) that "builds" do not actually always build the Image? i.e. sometimes they just retrieve a cached version. I know Eric mentioned that the docker build
prior art helps establish this pattern, otoh concepts in our API like force_build=True
imply that there's a meaningful distinction between building and hydrating from the cache.
(Also: I wonder if this method should have a force: bool
option 🤔).
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'm not sure if I like force
here. One can already force with all "recipe-oriented" API. Do you see build(force=True)
as a convenience for setting force_build
on every layer?
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.
Yeah I see it as a programmatic equivalent to using MODAL_FORCE_BUILD=1 ...
but maybe that's enough. Force-rebuilding the entire Image is actually often regretful because it makes every other Image that uses the same base rebuild too. Now that I think about it a little more, ad hoc application of MODAL_IGNORE_CACHE
is maybe more useful. But we can just drop it for now.
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 a little more I kinda like having ignore_cache: bool
here, though not critical. Could be nice in some Sandbox usecases where you want the process to always use some "fresh" version of some external image. Thought might require some work to wire it up since we don't currently have "ignore cache" in the API anywhere.
modal/image.py
Outdated
# Use this object_id with `Image.from_id` to use the built image in a sandbox or function. | ||
print(image.object_id) |
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'm not sure this example is sufficiently clear w/r/t how you would do that.
modal/image.py
Outdated
# No need to explicity build the image for defining a function. | ||
# with app.run(): | ||
# image.build(app) |
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 feels easily misread as giving them an example of the thing we don't want them to do 😁
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'm persuaded that image.build(app)
is ugly, but less ugly that existing alternatives for eagerly building an Image.
modal/image.py
Outdated
async def build(self, app: "modal.app._App") -> "_Image": | ||
"""Eagerly build an image. | ||
If your image was previously built, then this method will be a no-op and |
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.
Worried this is a little confusing since if you have an unhydrated Image object, we'll do an RPC and it's not technically a "no-op" it just doesn't rebuild.
modal/image.py
Outdated
"""Eagerly build an image. | ||
If your image was previously built, then this method will be a no-op and | ||
your built image is returned. |
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.
s/built/cached/? Not sure
modal/image.py
Outdated
built_image = modal.Image.from_id(my_image_id) | ||
``` | ||
Alternatively, you can re-built a image and use it in a sandbox. |
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.
Typo: re-built
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.
(Also I'm not sure what "re-build" would mean here. Do you mean pre-build?)
return obj | ||
|
||
async def build(self, app: "modal.app._App") -> "_Image": | ||
"""Eagerly build an image. |
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 a little more I kinda like having ignore_cache: bool
here, though not critical. Could be nice in some Sandbox usecases where you want the process to always use some "fresh" version of some external image. Thought might require some work to wire it up since we don't currently have "ignore cache" in the API anywhere.
Describe your changes
Although, eagerly building an image requires an app, I still think it's useful. In a future, where an app is not required, we can make the app kwarg optional.
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
image.build
to eagerly build an image: