Skip to content

Conversation

@cretz
Copy link
Member

@cretz cretz commented Jul 11, 2022

What was changed

  • Refactored to use callable protocols instead of callables where possible and moved into new temporalio.types package
  • Support registering activity methods and calling them via start_activity_method, execute_activity_method, start_local_activity_method, and execute_local_activity_method
  • Support registering activity classes and calling them via start_activity_class, execute_activity_class, start_local_activity_class, and execute_local_activity_class

Why?

Followup from #66 where we decided that instead of requiring fake instances each time you want to invoke an activity class/method, it's better to have first class support with overloads. We could not reasonably add overloads to the existing activity invocation functions without causing ambiguity that would give false negatives on bad types.

Checklist

  1. Closes [Feature Request] Support non-function callables as activities #52
  2. Closes [Feature Request] Inject dependencies into activities #68

@cretz cretz requested a review from a team July 11, 2022 16:40
@cretz cretz marked this pull request as ready for review July 11, 2022 18:23
Copy link
Member

@bergundy bergundy left a comment

Choose a reason for hiding this comment

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

I didn't read all of the overloads, mostly went over the tests and those seem fine to me.

I wish we didn't have to have execute_activity_class but I trust that you researched this and it's impossible to avoid.

@cretz
Copy link
Member Author

cretz commented Jul 14, 2022

I trust that you researched this and it's impossible to avoid.

I minimally researched it but I expect callable-class activities and execute_activity_class to be very rare. It's only for classes with __call__ which, at that point, why wouldn't you use a method? I am tempted to remove it altogether.

Blowing up the execute_activity overloads any more just for this rare case does not seem worth it.

@cretz cretz merged commit b8a80e0 into temporalio:main Jul 14, 2022
@cretz cretz deleted the activity-callable-instance branch July 14, 2022 14:56
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.

[Feature Request] Inject dependencies into activities [Feature Request] Support non-function callables as activities

2 participants