fix: raise GokartBuildError on FAILED_AND_SCHEDULING_FAILED status #465
Conversation
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
kitagry
requested review from
hirosassa,
Hi-king,
mski-iksm,
ujiuji1259 and
Copilot
There was a problem hiding this comment.
Pull Request Overview
This pull request enhances error handling in gokart by extending the set of statuses that trigger exceptions during task builds.
- Added new status checks (FAILED_AND_SCHEDULING_FAILED and SCHEDULING_FAILED) in the build function.
- Introduced a unit test to verify that a GokartBuildError is raised in the new error scenario.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/test_build.py | Added a unit test to validate error raising when the build result indicates failure due to scheduling issues. |
| gokart/build.py | Updated the status check in _build_task to include additional failure statuses. |
Comments suppressed due to low confidence (1)
test/test_build.py:212
- Consider adding an additional unit test case for when result.status is LuigiStatusCode.SCHEDULING_FAILED to ensure that both newly handled statuses are adequately covered.
with patch('luigi.build') as mock_luigi_build:
| @@ -204,7 +204,7 @@ def _build_task(): | |||
| ) | |||
| if task_lock_exception_raised.flag: | |||
| raise HasLockedTaskException() | |||
There was a problem hiding this comment.
[nitpick] Consider documenting the rationale for grouping the FAILED, FAILED_AND_SCHEDULING_FAILED, and SCHEDULING_FAILED statuses together. This makes the intention clearer, especially if their handling may diverge in the future.
Suggested change
| raise HasLockedTaskException() | |
| raise HasLockedTaskException() | |
| # Grouping statuses that represent failure scenarios. These statuses indicate that the task either failed during execution, | |
| # failed during scheduling, or both. They are treated similarly to trigger the same error handling logic. |
Copilot uses AI. Check for mistakes.
|
LGTM |
hiro-o918
approved these changes
Jun 5, 2025
| @@ -204,7 +204,7 @@ def _build_task(): | |||
| ) | |||
| if task_lock_exception_raised.flag: | |||
| raise HasLockedTaskException() | |||
| if result.status == luigi.LuigiStatusCode.FAILED: | |||
| if result.status in (LuigiStatusCode.FAILED, LuigiStatusCode.FAILED_AND_SCHEDULING_FAILED, LuigiStatusCode.SCHEDULING_FAILED): | |||
There was a problem hiding this comment.
Hi-king
reviewed
Jun 5, 2025
kitagry
force-pushed
the
catch-luigi-status
branch
from
42bbcb6 to
8c73d58
Compare
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 pull request enhances error handling in
gokartby extending the set of statuses that trigger exceptions during task builds and adding corresponding unit tests. The changes ensure more robust handling of scheduling failures and improve test coverage.