Skip to content

Report non-function Image build failures better #3172

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

Merged
merged 1 commit into from
May 27, 2025

Conversation

mwaskom
Copy link
Contributor

@mwaskom mwaskom commented May 20, 2025

Describe your changes

When Image builds fail, we try to print the result.exception. Unless the failing step is a .run_function, this will be empty, so we print an unhelpful message like

Image build for im-DEHjPnPEudJASdtbmWLtgW failed with the exception:\n'

This PR handles the empty exception case better, directing the user to the build logs.

In the case where output is not enabled, we add a hint suggesting modal.enable_output. Otherwise there won't be any obvious "build logs" for the user to refer to.

This is still a little unhelpful (i.e., in the case where the build failed without output enabled, you need to run the build again, and that might take a while). I think we can potentially capture more failure information from the worker and propagate it back to the client. But it will take some more changes.

  • Closes CLI-412
Checklists

Compatibility checklist

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

Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.


@mwaskom mwaskom requested a review from luiscape May 20, 2025 21:31
@mwaskom mwaskom force-pushed the michael/2025-05-20-better-image-build-failure branch from 969628f to 640d6e2 Compare May 27, 2025 18:25
f"RUN {requirements_prefix}{_get_modal_requirements_command(builder_version)}",
])
if "2024.10" >= builder_version > "2023.12":
modal_requirements_commands.extend(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this pulled in some unrelated ruff changes...

@mwaskom
Copy link
Contributor Author

mwaskom commented May 27, 2025

@ehdr suggested on Slack that the error message could include a link to app logs which is a nice idea — but I don't think we have immediate access to that URL without adding more global state to the Resolver, so will have to punt on it for now.

@mwaskom
Copy link
Contributor Author

mwaskom commented May 27, 2025

@prbot approve @ehdr

Copy link

@modal-pr-review-automation modal-pr-review-automation bot left a comment

Choose a reason for hiding this comment

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

Approved 👍. @ehdr will follow-up review this.

@mwaskom mwaskom merged commit 14bc5e1 into main May 27, 2025
26 checks passed
@mwaskom mwaskom deleted the michael/2025-05-20-better-image-build-failure branch May 27, 2025 20:33
@ehdr
Copy link
Contributor

ehdr commented May 27, 2025

LGTM!

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.

2 participants