Add a global default timeout for all wait() operations. #49
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
normandy7
reviewed
Oct 4, 2024
normandy7
approved these changes
Oct 7, 2024
michalsosn
reviewed
Oct 7, 2024
src/neptune_scale/__init__.py
Outdated
| timeout (float, optional): Maximum time in seconds to wait for the remaining operations to complete. | ||
| If the timeout is reached, `TimeoutError` is raised. | ||
| If the argument is not provided, the value of the `NEPTUNE_OPERATIONS_TIMEOUT_SECONDS` | ||
| environment variable is used. If the variable is not set, the default value of 1800 seconds is used. |
There was a problem hiding this comment.
there's no way to retry forever/disable timeout?
michalsosn
reviewed
Oct 7, 2024
| @@ -611,6 +627,11 @@ def _wait( | |||
| wait_condition.wait(timeout=wait_time) | |||
| value = wait_value.value | |||
|
|
|||
| timeout_left -= time.monotonic() - t0 | |||
There was a problem hiding this comment.
why are you doing it this way instead of the usual (at least I've seen this pattern in this codebase)
start_time = time.monotonic()
while True:
..
time_elapsed = time.monotonic() - start_time
if time_elapsed > timeout:
boom
?
Your implementation may wait longer than the specified timeout, because you are moving the t0 on each loop and thus ignoring the time of executing BLOCK B
while True:
t0 = time.monotonic() ## stores e.g. 1000ms, 2000ms, 3000ms on each iteration
BLOCK A # ~900ms passes
timeout_left = time.monotonic() - t0 ## subtracts e.g. 1900ms - 1000ms, 2900 - 2000, 3900 - 3000...
BLOCK B # ~100ms passes, but it's not subtracted from timeout_left
The timeout is configurable via `NEPTUNE_OPERATIONS_TIMEOUT_SECONDS` env variable. `Run.close()` now accepts an optional `timeout` argument as well.
kgodlewski
force-pushed
the
kg/default-timeout-for-wait-operations
branch
from
58ca5a0 to
d6397b5
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.
The timeout is configurable via
NEPTUNE_OPERATIONS_TIMEOUT_SECONDSenv variable. If not provided via the env or method arguments, the default timeout is 300 sec.Run.close()now accepts an optionaltimeoutargument as well.Important
Note that this PR changes the semantics of
timeoutmaking it raiseTimeoutErrorinstead of simply returning from await*()method. Previously thetimeoutargument wouldn't really do anything useful.