-
Notifications
You must be signed in to change notification settings - Fork 0
Fix task failure handling when task is not a Signature #56
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
Fix task failure handling when task is not a Signature #56
Conversation
…ise mageflwo error
…est to check that normal fail task raises the correct error message
…f task is normal task, if true raise error noramlly
…run return message as is
…t, we check the normal output is returned
…ted a task_fail in invoker, merge remove
…as fail after publish error
…d fail is called after callbacks. rename to task success
…task is not needed
… need to update signature models
…ests for normal tasks
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.
Pull request overview
This PR fixes a critical bug where normal (non-signature) tasks did not properly propagate errors and results. The implementation now correctly differentiates between signature-based tasks and vanilla task runs.
Changes:
- Added
is_vanilla_run()method to detect when tasks are run without signatures (no task_id) - Implemented proper exception handling for normal tasks that raises exceptions immediately without attempting signature-related operations
- Changed normal tasks to return results directly without unnecessary
HatchetResultwrapping - Simplified and clarified the invoker API by renaming methods from
run_success/run_error/remove_tasktotask_success/task_failed - Removed unused code (
SIGNATURES_NAME_MAPPINGandupdate_register_signature_models) - Reordered the callback flow so
signature.done()/signature.failed()are called after task lifecycle callbacks complete - Added comprehensive test coverage for normal task failures, retries, cancellations, and timeouts
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| mageflow/callbacks.py | Updated task callback handler to detect vanilla runs and handle them separately; reordered signature status updates to occur after task lifecycle callbacks |
| mageflow/invokers/base.py | Simplified abstract base class by renaming methods to clearer names (task_success/task_failed) |
| mageflow/invokers/hatchet.py | Implemented is_vanilla_run() method; renamed and refactored task lifecycle methods; removed unused remove_task method |
| mageflow/signature/model.py | Removed unused SIGNATURES_NAME_MAPPING global variable |
| mageflow/startup.py | Removed unused update_register_signature_models function and its call from init_mageflow |
| tests/unit/conftest.py | Removed call to deleted update_register_signature_models function; minor formatting improvement |
| tests/integration/hatchet/worker.py | Changed fail_task to raise MageflowTestError for clearer test assertions; added normal_retry_once task for testing normal task retry behavior |
| tests/integration/hatchet/models.py | Added MageflowTestError exception class for testing |
| tests/integration/hatchet/assertions.py | Removed assert_task_done helper function that was specific to wrapped task results |
| tests/integration/hatchet/signature/test_edge_case.py | Added four new test cases for normal task failure, retry, cancel, and timeout scenarios |
| tests/integration/hatchet/signature/test__signature.py | Reduced sleep times in tests for faster execution; updated assertions to check task status directly for normal tasks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… return types for invoker
Summary
Fixes bug where normal (non-signature) tasks did not properly propagate errors and results. When a task was run without a signature, the error handling logic would incorrectly attempt to update non-existent signature state, and successful results were wrapped unnecessarily.
Closes #54
Changes
is_vanilla_run()method toHatchetInvokerto detect normal task runs (no task_id)HatchetResultwrappingrun_success/run_error/remove_taskwithtask_success/task_failedSIGNATURES_NAME_MAPPINGandupdate_register_signature_models()functionsignature.done()/signature.failed()now called after task lifecycle callbacksTesting
test_check_normal_task_fails__sanity)test_retry_normal_tasks__sanity)test_cancel_task_output__sanity)test_timeout_task_output__sanity)