Skip to content

Add execute and await completion function#643

Merged
alb-rl merged 2 commits intomainfrom
alb/add-execute-and-await-completion
Sep 11, 2025
Merged

Add execute and await completion function#643
alb-rl merged 2 commits intomainfrom
alb/add-execute-and-await-completion

Conversation

@alb-rl
Copy link
Copy Markdown
Contributor

@alb-rl alb-rl commented Sep 10, 2025

New execute_and_await_completion does execution and completion all in one.

We now rely on uuid-utils to generate uuidv7 for each command, making them idempotent. Since that library requires python version >= 3.9, I'm removing support for 3.8

Also discovered the our polling mechanism doesn't handle timeouts well (thank you smoketests). Will fix in a follow-up

Copy link
Copy Markdown
Contributor

@tode-rl tode-rl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small suggestions but looks good; is not supporting 3.8 breaking or doesn't matter bc it's just for our library?

assert isinstance(received, str)


@pytest.mark.timeout(30)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test that doesn't use pollingConfig to make sure basecase is ok?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessary; the original test won't even use any of the polling config parameters

Comment on lines +827 to +835
def handle_timeout_error(error: Exception) -> DevboxAsyncExecutionDetailView:
if isinstance(error, APITimeoutError) or (
isinstance(error, APIStatusError) and error.response.status_code == 408
):
return execution
raise error

def is_done(result: DevboxAsyncExecutionDetailView) -> bool:
return result.status == "completed"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are re-used between the two functions, extract out?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this, but poll_until requires a Callable with one argument. If I extract out, I would need two arguments, the error and the execution.

Then I'd have to define another helper function to do call that extracted function with the execution, which isn't good. I'd rather keep on_error generic with only one argument

extra_query: Query | None = None,
extra_body: Body | None = None,
timeout: float | httpx.Timeout | None | NotGiven = NOT_GIVEN,
idempotency_key: str | None = None,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth instantiating idempotency_key -> command_id = str(uuid7()) within fn? idk if too much

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

command_id is for internal use; idempotency_key should be specified by the user

@alb-rl alb-rl merged commit e60ad46 into main Sep 11, 2025
7 checks passed
@alb-rl alb-rl deleted the alb/add-execute-and-await-completion branch September 11, 2025 00:06
@alb-rl
Copy link
Copy Markdown
Contributor Author

alb-rl commented Sep 11, 2025

is not supporting 3.8 breaking or doesn't matter bc it's just for our library?

It should be fine. Most if not all users are using python >= 3.9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants