fix(executor): attribute parallel-batch exceptions to the correct action (#415)#417
Open
Yuthika10 wants to merge 1 commit into
Open
Conversation
Contributor
Author
|
Hi @VyomKulshrestha just sent a PR for issue #415 - has passed all 6 checks. Can you please review the code? Would love your feedback on this for any further improvements. |
Contributor
Author
|
Hi @VyomKulshrestha! Can you plese go thorugh this PR? |
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.
Summary
In the parallel-batch path of
Executor.execute, when an action raised an exception, the failure was always recorded againstbatch[0]- the first action in the batch - regardless of which action actually raised.asyncio.gatherreturns results positionally, so the exception's position inbatch_resultscorresponds tobatch[position], but that position was discarded andbatch[0]was hardcoded. Since_analyze_dependenciesgroups independent actions into multi-action batches, a non-first action raising would name the wrong action in the resultingActionResultand the audit trail.This fixes the attribution to use the position, and extracts the batch-result collection into a small testable method so the behavior can be unit-tested.
Closes #415
Type of change
Changes made
daemon/pilot/agents/executor.py: extracted the parallel-batch result handling into_collect_batch_results(batch, batch_results, results), which iterates withenumerateand attributes a raised exception tobatch[position]instead ofbatch[0]. The success branch is unchanged (it already used theidxreturned byexecute_single_action).daemon/tests/test_executor_batch_attribution.py: new unit tests - a non-first action raising is attributed to the correct action, the first-action case still works (guards against simply flipping the bug), and an all-success batch reports no failure.How to test
pytest daemon/tests/test_executor_batch_attribution.py -v- the three tests pass.test_exception_attributed_to_failing_action_not_first, fails against the oldbatch[0]code and passes with the fix - it builds a 3-action batch where the second action raises and asserts the failure names that action.ruff checkandruff format --checkon the two files are clean.Checklist
pytestfor backend,npm run testfor frontend)Screenshots / recordings (if UI change)
N/A — no UI change.
GSSoC declaration