Deal with errors better#7
Open
neil-hiverge wants to merge 3 commits into
Open
Conversation
hiverge-robot
added
needs-triage
Collaborator
Author
|
/kind cleanup |
There was a problem hiding this comment.
Pull request overview
This PR improves how evaluator execution failures are surfaced to API callers by refining run_command error reporting and avoiding accidental double-wrapping of execution errors.
Changes:
- Add
last_output_lines(and a max-tail constant) to include a focused tail of evaluator output in non-zero-exit errors. - Update
run_commandto distinguish signal termination vs non-zero exit, handle “no output” cleanly, and improve the timeout message. - Update
execute_python_functionto re-raise the originalFunctionExecutionErrorinstead of wrapping it, and add a newcommon_toolstest suite.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| main.py | Stops wrapping FunctionExecutionError again when checkpoint retrieval fails. |
| common_tools.py | Adds output-tail helper and improves run_command error handling/messages. |
| common_tools_test.py | Introduces tests covering success, non-zero exit, signal termination, timeout, and output-tail behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+56
to
+57
| source = stderr if stderr.strip() else stdout | ||
| return "\n".join(source.strip().splitlines()[-max_lines:]) |
| lines = stdout.strip().splitlines() | ||
| if not lines: | ||
| # Exited cleanly but printed nothing, so there is no result to parse. | ||
| raise FunctionExecutionError("Evaluator Format Error: No output was written.") |
| except subprocess.TimeoutExpired as exc: | ||
| process.kill() | ||
| raise FunctionExecutionError("Timeout") from exc | ||
| raise FunctionExecutionError(f"Evaluation timed-out after {timeout} seconds.") from exc |
Comment on lines
+74
to
+77
| """ | ||
| A command killed by a signal reports the signal with an execution-failed | ||
| prefix. | ||
| """ |
hiverge-robot
added
cleanup
Collaborator
Author
|
/hold |
hiverge-robot
added
the
do-not-merge/hold
kerry-hiverge
approved these changes
Jul 10, 2026
kerry-hiverge
left a comment
kerry-hiverge
left a comment
Contributor
There was a problem hiding this comment.
lgtm! one small comment
| the evaluator produced no output at all. | ||
| """ | ||
| source = stderr if stderr.strip() else stdout | ||
| return "\n".join(source.strip().splitlines()[-max_lines:]) |
Contributor
There was a problem hiding this comment.
i think it could be useful to show both the stderr and stdout if both are present, e.g., the stdout could give useful context about where the program was up to before it crashed. we could present the last few lines of both if both are nonempty perhaps
Collaborator
Author
|
Clean test run now: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR improves the error handling in the daemon server.
See also this PR: https://github.com/hiverge/hiverge/pull/553