Skip to content

Expense example #201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Expense example #201

wants to merge 8 commits into from

Conversation

jssmith
Copy link
Contributor

@jssmith jssmith commented Jun 23, 2025

What was changed

Ported the expense example from Go to Python

Why?

Demonstrate human-in-the-loop processing and asynchronous activity completion

Checklist

  1. Closes N/A

  2. How was this tested:
    Includes tests

  3. Any docs updates needed?
    README is updated

@jssmith jssmith requested a review from a team as a code owner June 23, 2025 16:47
@jssmith jssmith requested a review from tconley1428 June 23, 2025 16:47
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Only minor things, nothing technically blocking

if not expense_id:
raise ValueError("expense id is empty")

async with httpx.AsyncClient() as client:
Copy link
Member

Choose a reason for hiding this comment

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

Consider making the activities as methods on a class that instantiates this in __init__ and reuses it. It is a bit better performance to reuse an HTTP client/connection and I think a better practice to demonstrate to users

f"{EXPENSE_SERVER_HOST_PORT}/create",
params={"is_api_call": "true", "id": expense_id},
)
response.raise_for_status()
Copy link
Member

Choose a reason for hiding this comment

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

Technically 4xx is probably a ApplicationError with non_retryable=True, but that's a bit pedantic. But with this setup, an activity that, say, has invalid auth will retry forever

#
# Activity completion is signaled in the `notify_expense_state_change`
# function in `ui.py`.
activity.raise_complete_async()
Copy link
Member

Choose a reason for hiding this comment

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

I see 8 years ago in the Go samples we used async activity completion. But another alternative that may be more popular these days is sending a signal on completion. See https://docs.temporal.io/activity-execution#when-to-use-async-completion. In this case, unless that external call is heartbeating and can react to cancellation, signal may be best. But not a big deal.

start_to_close_timeout=timedelta(seconds=10),
)
except Exception as err:
logger.error(f"Failed to create expense report: {err}")
Copy link
Member

Choose a reason for hiding this comment

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

Change to logger.exception if you want the exception info like message and stack trace automatically added


[tool.uv]
default-groups = [
"dev",
"bedrock",
"dsl",
"encryption",
"expense",
Copy link
Member

Choose a reason for hiding this comment

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

We accidentally added these default groups and we will remove them in #190

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