Skip to content
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

Encourage threaded activities, warn when max_workers too low, and other small changes #387

Merged
merged 4 commits into from
Oct 6, 2023

Conversation

cretz
Copy link
Member

@cretz cretz commented Sep 25, 2023

What was changed

  • Prefer synchronous threaded activities over async ones in README
  • Document that clients and runtimes do no work across forks
  • Warn when max_workers on activity executor is not as high as max_concurrent_activities
  • Re-enabled some patch tests previously disabled

Checklist

  1. Closes [Feature Request] Default to suggesting threaded activities (and other async helper docs) #358
  2. Closes [Bug] Client stuck connecting or starting a workflow #385

@cretz cretz requested a review from a team as a code owner September 25, 2023 13:55
@MasonEgger
Copy link

MasonEgger commented Sep 25, 2023

Just my two cents here, take it or leave it.

I personally don't think we should be pushing the Python engineers into Thread Pools for various reasons:

  1. Thread Pools can get nasty quick. I don't know how much of the complexity of the Thread Pool we expose, but if we think that Python devs will know more about Thread Pools than AysncIO we're wrong in that assumption.
    1. I guarantee you no more than 1% of Python devs have ever actually worked with a Thread Pool.
    2. We will still see deadlocks
  2. The Python community isn't accustomed to thinking in an async-first way, however, they 100% need to get there. This is kind of the "Be the change you want to see in the world" kind of thing. I would recommend async first and make sure we are properly educating them than reverting back to the sync model.
  3. From an education point of view it is easier for us to say "Hey, don't make these types of calls inside your async functions or bad things will happen" rather than "Let's teach you about the joys of threading in Python"
    1. Anytime you have to explain the GIL, you done messed up
    2. I'm curious about how all this GIL removal work, which is gaining serious traction, will affect this.

Again, my 2 cents, take it as you will. The simplicity demonstrated via implementing Workflows and Activities via async is :chefskiss: and very pythonic. The thread pool executor is, to me, not as elegant and hard to say we've made your code simpler.

@cretz
Copy link
Member Author

cretz commented Sep 25, 2023

Discussed off-PR, but basically:

  • It is easier to opt-in to async than to know you need to opt out
  • Many users bring existing code which is not async compatible
  • Users cannot reasonably tell whether all transitive invocations are async-capable
  • Companies are forbidding async def activities because it's so hard to tell things are really async
  • Thread pool executor is created by users but we use it, users don't have to concern themselves w/ threading
  • Getting the async choice wrong will only surface in runtime, and it can gum up the system in not-so-obvious ways
  • Users/support are constantly just telling users, as a solution, to just stop using async
  • It is unreasonable to burden operators/support with these problems while we wait for some idealistic future where Python developers better understand async
  • This just changes the README quickstart and a couple of other README pieces

@cretz cretz merged commit ded3747 into temporalio:main Oct 6, 2023
12 checks passed
@cretz cretz deleted the sync-preferred branch October 6, 2023 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants