Skip to content
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

run_function return type specificity #2428

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thundergolfer
Copy link
Contributor

Describe your changes

context: https://modal-com.slack.com/archives/C06UKEPF3HR/p1730314009504369

  • Provide Linear issue reference (e.g. MOD-1234) if available.
Backward/forward compatibility checks

Just a type-checking change and docs change.


Check these boxes or delete any item (or this section) if not relevant for this PR.

  • Client+Server: this change is compatible with old servers
  • Client forward compatibility: this change ensures client can accept data intended for later versions of itself

@@ -1396,7 +1396,7 @@ def build_dockerfile(version: ImageBuilderVersion) -> DockerfileSpec:

def run_function(
self,
raw_f: Callable[..., Any],
raw_f: Callable[..., None],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a narrowing but I can't think of a case where returning something is desired. These should just be side-effect functions.

@mwaskom
Copy link
Contributor

mwaskom commented Oct 30, 2024

I could imagine users doing something like this

def load_model_with_caching():
    return transformers.AutoModel.from_pretrained("whatever-bert")


image = modal.Image.debian_slim().run_function(load_model_with_caching)

...

@app.function()
def do_inference(input):
    model = load_model_with_caching()
    model(input)

Similar to our advertised "stack @modal.build / @modal.enter pattern, to take advantage of huggingface's caching during the Image build.

Still it's helpful to clarify that we don't use the return value. Maybe that could just be done with narrative documentation? Not sure.

@thundergolfer
Copy link
Contributor Author

Yeh I think I've even done something similar myself. But the problem is that in the run_function case Modal's client will actually try pickle that return value and send it as an output, which is undesirable.

@mwaskom
Copy link
Contributor

mwaskom commented Oct 30, 2024

But the problem is that in the run_function case Modal's client will actually try pickle that return value and send it as an output, which is undesirable.

I guess it wouldn't be trivial not to do this since the container doesn't know it's running a "build function"?

@thundergolfer
Copy link
Contributor Author

I guess it wouldn't be trivial not to do this since the container doesn't know it's running a

It might actually be easy enough. I think the container can access the Function definition which has something like is_build_function

@freider
Copy link
Contributor

freider commented Oct 31, 2024

Good point, if it's not too hard structurally in the container entrypoint we should add something that just returns None as the result value when running a builder function! I've ran into this issue where builds take ages because of doing things like run_function(load_model_with_caching) like above to reuse the same model loading function without declaring a separate builder function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants