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

Remove old task runners and futures modules #13593

Merged
merged 15 commits into from
May 29, 2024
Merged

Conversation

desertaxle
Copy link
Member

@desertaxle desertaxle commented May 28, 2024

Moves new_task_runners and new_futures to task_runners and futures, respectively. Deletes old task_runners and future modules.

Example

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • This pull request includes a label categorizing the change e.g. maintenance, fix, feature, enhancement, docs.

For documentation changes:

  • This pull request includes redirect settings in netlify.toml for files that are removed or renamed.

For new functions or classes in the Python SDK:

  • This pull request includes helpful docstrings.
  • If a new Python file was added, this pull request contains a stub page in the Python SDK docs and an entry in mkdocs.yml navigation.

@@ -24,7 +24,7 @@


async def main(timeout):
async with anyio.move_on_after(timeout):
with anyio.move_on_after(timeout):
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes the failures we've been seeing with the benchmarks job

@desertaxle desertaxle marked this pull request as ready for review May 29, 2024 14:05
@desertaxle desertaxle requested review from zzstoatzz and a team as code owners May 29, 2024 14:05
Copy link
Collaborator

@abrookins abrookins left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍

@abrookins
Copy link
Collaborator

One question I have that isn't directly about this PR but that it reminded me of is why did we remove the SequentialTaskRunner? I get that it doesn't serve a purpose in production code, but I thought it was there to help people write tests for their flows easier. 🤔

@desertaxle
Copy link
Member Author

One question I have that isn't directly about this PR but that it reminded me of is why did we remove the SequentialTaskRunner? I get that it doesn't serve a purpose in production code, but I thought it was there to help people write tests for their flows easier. 🤔

I removed it because the SequentialTaskRunner breaks the model of .submit triggering a non-blocking execution of a task. I would suggest the ThreadPoolTaskRunner to users if they want to test their flows since it maintains non-blocking execution, but you can run it locally without any dependencies.

@desertaxle desertaxle merged commit 0d89908 into main May 29, 2024
28 of 33 checks passed
@desertaxle desertaxle deleted the clean-up-old-task-runners branch May 29, 2024 14:47
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.

None yet

2 participants