-
Notifications
You must be signed in to change notification settings - Fork 20
fix: improve eval resume flow consistency and add validation #1177
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
base: main
Are you sure you want to change the base?
Conversation
4bba0d8 to
a61549e
Compare
Previously, the eval flow only passed UiPathExecuteOptions when resume=True, but passed no options at all when resume=False. This is inconsistent with the debug flow (cli_debug.py) which always explicitly passes the options. This change: - Makes the eval flow always pass UiPathExecuteOptions explicitly - Simplifies the if/else logic by separating input determination from execution - Ensures consistency across both debug and eval commands - Makes the resume=False intent explicit rather than relying on default behavior While functionally equivalent (execute() accepts options=None and defaults resume to False), this change improves code maintainability and explicitness.
Add comprehensive unit tests to verify that UiPathExecuteOptions is always passed explicitly in the eval flow, matching the debug flow pattern. Tests verify: - UiPathExecuteOptions(resume=False) is passed when resume=False - UiPathExecuteOptions(resume=True) is passed when resume=True - Options are NEVER None, always explicit - Behavior is consistent with debug flow Uses mocking to directly test the execute_runtime method, ensuring the specific code path we modified is properly tested.
a61549e to
c336c36
Compare
akshaylive
left a comment
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.
How would this work when num_workers > 0? If it isn't supposed to work, we need to alert it somehow and halt execution. In other words, we may want to pass the resume to only the runtime instance that had previously suspended, and not to all runtime instances.
It won't work for more than 1, its a limitation of suspend and resume. We can raise an error. |
- Add ValueError when resume mode is used with multiple evaluations - Validates early in initiate_evaluation() before expensive operations - Provides clear error message with guidance to use --eval-ids - Add test coverage with new multiple-evals.json fixture - Add test_resume_with_multiple_evaluations_raises_error() test
akshaylive
left a comment
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.
🚢
Problem
The eval flow had two issues:
Inconsistency in handling resume options compared to the debug flow:
cli_debug.py): Always explicitly passesUiPathExecuteOptions(resume=resume)regardless of the resume value_runtime.py): Only passedUiPathExecuteOptions(resume=True)when resume=True, but passed no options at all when resume=FalseMissing validation for the unsupported scenario of running multiple evaluations with resume mode enabled
Code Comparison (Issue #1)
Before (eval flow):
Debug flow (correct pattern):
Issue #2: Missing Validation
Resume mode relies on checkpoint discovery using a consistent
thread_id. When multiple evaluations run in parallel, each creates separate runtime contexts, making it impossible to determine which checkpoint to resume from. This was not being caught, potentially leading to confusing runtime behavior.Solutions
1. Consistent Options Passing
Made the eval flow consistent with the debug flow by:
UiPathExecuteOptionsexplicitly, regardless of resume valueAfter:
2. Resume Mode Validation
Added early validation in
initiate_evaluation()to catch the unsupported scenario:Benefits:
Testing
test_resume_with_multiple_evaluations_raises_error()to verify validationmultiple-evals.jsontest fixture with 2 evaluationsImpact