Skip to content

Conversation

@cretz
Copy link
Member

@cretz cretz commented Feb 17, 2023

What was changed

  • Disallow non-instances as activities
  • Add proper query list to end of exception
  • Make "None" result as null for async activity completion
  • Improve workflow sandbox suggestions
    • Update quick start to suggest putting workflows in separate file
    • Improve clarity of error message on restricted workflow access
    • Add above error message to README for better SEO
  • Clarify that heartbeat_timeout is needed on caller side when wanting heartbeat/cancellation for activities (even if not technically required)

Checklist

  1. Closes [Feature Request] Eagerly error when attempting to register a callable activity class instead of its instance #228
  2. Closes [Feature Request] Properly list queries in query-handler-not-found error #265
  3. Closes [Feature Request] Confirm async activity completion can give null value for Optional-return activity #266
  4. Closes [Feature Request] Improve workflow sandboxing suggestions #273
  5. Closes [Feature Request] Clarify requirement of heartbeat timeout wrt cancellation #282

@cretz cretz requested a review from a team February 17, 2023 15:07

```

This assumes there's an activity in `my_activities.py` like:
Copy link
Member

Choose a reason for hiding this comment

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

nit: Why do activities need the my_ prefix and workflows don't?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't specify a workflow file name in this section. If you're talking about the quick start section, neither needs a my_ prefix. I chose to add it here for clarity since it's not an end-to-end example.

Comment on lines +1141 to +1142
In order for a non-local activity to be notified of cancellation requests, it must be given a `heartbeat_timeout` at
invocation time and invoke `temporalio.activity.heartbeat()` inside the activity. It is strongly recommended that all
Copy link
Member

Choose a reason for hiding this comment

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

That's not true, you don't have to specify a heartbeat_timeout in order to be cancellable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know that it's not strictly true but the sentence reads better with "must X and Y" than "must Y and should X".

function regularly. "Types of Activities" has specifics on cancellation for asynchronous and synchronous activities.
In order for a non-local activity to be notified of cancellation requests, it must be given a `heartbeat_timeout` at
invocation time and invoke `temporalio.activity.heartbeat()` inside the activity. It is strongly recommended that all
but the fastest executing activities call this function regularly. "Types of Activities" has specifics on cancellation
Copy link
Member

Choose a reason for hiding this comment

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

nit: you might want to mention throttling here, up to you.

f"Cannot access {qualified_name} from inside a workflow. "
"If this is code from a module not used in a workflow or known to "
"only be used deterministically from a workflow, mark the import "
"as pass through."
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd consider linking to some doc that provides more information.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no stable link I can provide here

@cretz cretz merged commit c6e204a into temporalio:main Feb 21, 2023
@cretz cretz deleted the minor-updates branch February 21, 2023 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment