fix: remove timeout from Invoke Config#340
Conversation
|
thank you very much @AdityaAudi! I've flagged this for v2 with milestone. |
AdityaAudi
changed the title
|
No longer mergeable due to merge conflicts. |
@zhongkechen I took a look and I've resolved the conflicts |
|
Thanks @AdityaAudi, change looks good. |
github-project-automation
Bot
moved this from Backlog
to Pending merge
in aws-durable-execution
ayushiahjolia
dismissed
zhongkechen’s stale review
We are adding breaking changes in v2 branch.
Isn't |
|
After internal discussions, we decided to keep breaking changes in main and separate v1.x branch for SDK 1.x version. |
AdityaAudi
force-pushed
the
fix/remove-invoke-config-timeout
branch
from
13e5c3f to
efcbca0
Compare
Thanks @ayushiahjolia! I've rebased onto the current main ( |
|
|
||
| When no immediate response, operation should suspend with timeout. | ||
| """ | ||
| def test_invoke_immediate_response_no_immediate_response(): |
There was a problem hiding this comment.
There was a problem hiding this comment.
Good catch, thanks @ayushiahjolia, CI flagged the same thing (F811). The duplicates came from the original PR commit copy-pasting a subset of the immediate-response tests, which later collided with the same-named tests that landed on main. Python was silently overwriting the earlier definitions, so only the shorter duplicates were actually running.
I've pushed a follow-up commit that drops the PR-added copies, so the more thorough versions on main (lines 817 and 936) now execute as intended. Lint is clean locally and all 42 tests in invoke_test.py still pass. Let me know if you'd like me to squash before merge.
| config = InvokeConfig[str, str]() | ||
|
|
||
| with pytest.raises(TimedSuspendExecution): | ||
| with pytest.raises(SuspendExecution): |
There was a problem hiding this comment.
pytest.raises(SuspendExecution) does not prove the change.
TimedSuspendExecution is a subclass of SuspendExecution (exceptions.py:330), so this assertion would still pass if we regressed to raising TimedSuspendExecution.
suggestion:
with pytest.raises(SuspendExecution) as exc_info:
invoke_handler(...)
assert type(exc_info.value) is SuspendExecution # exact class, not subclass
There was a problem hiding this comment.
this is a nit :-) not blocking
Address review feedback: pytest.raises(SuspendExecution) alone would still pass if we regressed to TimedSuspendExecution, since it is a subclass. Assert the exact class instead.
github-project-automation
Bot
moved this from Pending merge
to Done
in aws-durable-execution
|
thank you so much for your patience and for your contribution @AdityaAudi, this one had to wait a while for the major version increment :-) your change is the very 1st v2 change merged into the trunk 🥳 |
|
Thank you @yaythomas! Really appreciate the thoughtful reviews from you and @ayushiahjolia throughout, glad we got this one landed. Honored to have it be the first v2 change into trunk 🎉 Looking forward to contributing more. |
5 participants
Breaking change: Customers passing
timeout=toInvokeConfigwill receive aTypeErroron upgrade. Targeting v2!Summary
Removes the unused
timeoutfield fromInvokeConfigas described in #321The field was passed to
suspend_with_optional_resume_delayas a polling retry delay, but this has no meaningful effect, invoke completion is pushed by the backend, not polled. The field was misleadingly named and did not enforce any actual timeout on the invocation.Changes
config.py- removedtimeout: Durationfield andtimeout_secondsproperty fromInvokeConfigoperation/invoke.py-suspend_with_optional_resume_delaycalled without delay (indefinite suspend)tests/- removedDurationimport, removed/updated all timeout-specific invoke testsdocs/architecture.md- removedtimeout_secondsfromInvokeConfigclass diagramdocs/core/invoke.md- removed all timeout references (terminology, config, error handling, best practices, FAQ, testing)Test plan
hatch test -- -p no:randomly)timeout=incidentally now use defaultInvokeConfig()