don't override timeout on with_overrides if not specified #3097
Conversation
Signed-off-by: Paul Dittamo <[email protected]>
Code Review Agent Run Status
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3097 +/- ##
===========================================
- Coverage 76.46% 46.74% -29.73%
===========================================
Files 218 203 -15
Lines 22505 21525 -980
Branches 2766 2779 +13
===========================================
- Hits 17209 10062 -7147
- Misses 4483 10967 +6484
+ Partials 813 496 -317 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Paul Dittamo <[email protected]>
pvditt
requested review from
wild-endeavor,
kumare3,
eapolinario,
pingsutw,
cosmicBboy,
samhita-alla,
thomasjpfan and
Future-Outlier
as code owners
Code Review Agent Run #01a35dActionable Suggestions - 3
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
flytekit/core/node.py
Outdated
| @@ -47,6 +47,8 @@ class Node(object): | |||
| ID, which from the registration step | |||
| """ | |||
|
|
|||
| timeout_override_sentinel = object() | |||
There was a problem hiding this comment.
Consider if using object() as a sentinel value is the best approach here. While it works, using a dedicated sentinel class or enum.Enum could provide better type safety and clarity of intent.
Code suggestion
Check the AI-generated fix before applying
import datetime
+import enum
@@ -50,1 +50,4 @@
-timeout_override_sentinel = object()
+class TimeoutSentinel(enum.Enum):
+ NO_OVERRIDE = 'no_override'
+
+timeout_override_sentinel = TimeoutSentinel.NO_OVERRIDE
Code Review Run #01a35d
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| raise ValueError("timeout should be duration represented as either a datetime.timedelta or int seconds") | ||
| if timeout is not Node.timeout_override_sentinel: | ||
| if timeout is None: | ||
| node_metadata._timeout = 0 |
There was a problem hiding this comment.
Consider using datetime.timedelta() instead of 0 for timeout value to maintain consistency with the type system and other code paths.
Code suggestion
Check the AI-generated fix before applying
| node_metadata._timeout = 0 | |
| node_metadata._timeout = datetime.timedelta() |
Code Review Run #01a35d
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| t1_expected_timeout_overridden, | ||
| t1_expected_timeout_unset, | ||
| t2_expected_timeout_overridden, | ||
| t2_expected_timeout_unset, | ||
| ): |
There was a problem hiding this comment.
Consider using more descriptive parameter names. The current names t1_expected_timeout_overridden, t1_expected_timeout_unset, etc. could be renamed to better indicate their purpose, such as task1_timeout_with_override and task1_timeout_without_override.
Code suggestion
Check the AI-generated fix before applying
| t1_expected_timeout_overridden, | |
| t1_expected_timeout_unset, | |
| t2_expected_timeout_overridden, | |
| t2_expected_timeout_unset, | |
| ): | |
| task1_timeout_with_override, | |
| task1_timeout_without_override, | |
| task2_timeout_with_override, | |
| task2_timeout_without_override, | |
| ): |
Code Review Run #01a35d
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
flytekit/core/node.py
Outdated
| @@ -47,6 +47,8 @@ class Node(object): | |||
| ID, which from the registration step | |||
| """ | |||
|
|
|||
| timeout_override_sentinel = object() | |||
There was a problem hiding this comment.
Nit: I usually see python sentinels as constants, thus all capitalized:
| timeout_override_sentinel = object() | |
| TIMEOUT_OVERRIDE_SENTINEL = object() |
There was a problem hiding this comment.
sounds good - just updated
Signed-off-by: Paul Dittamo <[email protected]>
Signed-off-by: Paul Dittamo <[email protected]>
Signed-off-by: Paul Dittamo <[email protected]>
Code Review Agent Run #7ddfdbActionable Suggestions - 0Review Details
AI Code Review powered by |
Tracking issue
fixes: flyteorg/flyte#6153
Why are the changes needed?
with_overrides set timeout to default of 0 if timeout param is not specified
What changes were proposed in this pull request?
How was this patch tested?
added unit tests
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This PR fixes a critical bug in the with_overrides() method where timeouts were incorrectly defaulting to 0. The implementation introduces a sentinel object (renamed to TIMEOUT_OVERRIDE_SENTINEL for Python naming conventions) to distinguish between explicit timeout settings and unspecified cases. The changes modify the Node class timeout handling logic and improve differentiation between unset timeout and explicit None timeout cases, with comprehensive test coverage for timeout override scenarios.Unit tests added: True
Estimated effort to review (1-5, lower is better): 2