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

Codemod to fix asyncio.Task #248

Merged
merged 8 commits into from
Feb 16, 2024
Merged

Codemod to fix asyncio.Task #248

merged 8 commits into from
Feb 16, 2024

Conversation

clavedeluna
Copy link
Contributor

@clavedeluna clavedeluna commented Feb 9, 2024

Overview

Replace manual init of asyncio.Task with create_task

Description

  • special case of loop=not-a-loop in which I left without changes, I can update accordingly if we do want to change it.

@clavedeluna clavedeluna force-pushed the task-init branch 2 times, most recently from af18bad to 5bdc3e8 Compare February 9, 2024 12:27
@clavedeluna clavedeluna marked this pull request as ready for review February 9, 2024 21:48
Copy link
Contributor

@andrecsilva andrecsilva left a comment

Choose a reason for hiding this comment

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

Just a small correction on converting tasks with eager_start parameter.

Comment on lines 123 to 130
"""
import asyncio
asyncio.Task(coro(1, 2), name='task', eager_start=True)
""",
"""
import asyncio
asyncio.create_task(coro(1, 2), name='task', eager_start=True)
""",
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the intended behavior of the code. The eager_start argument has seemingly no effect on create_task.

I think the ayncio.eager_task_factory is the intended way to create eager tasks.
https://docs.python.org/3/library/asyncio-task.html#eager-task-factory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah interesting. I only used eager_task as a kwarg test example to demonstrate we correctly handle kwargs. I'll see if I use another kwarg or if we need to special case for eager_task

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrecsilva could you double check my understanding of the docs, since it's a tiny bit confusing. Seems like eager_start is new so not a ton of use cases yet out there.

So if I understand correctly, if you want to create a Task() with eager_start=True, the code would be

loop = asyncio.get_running_loop()
task = asyncio.Task(my_coroutine(), loop=loop, eager_start=True)

(must pass in a loop otherwise it won't work)

Our replacement could either be

  loop = asyncio.get_running_loop()
  loop.set_task_factory(asyncio.eager_task_factory)
  task = loop.create_task(my_coroutine())

which is kinda what the docs suggest? but this also works:

  loop = asyncio.get_running_loop()
  task = asyncio.eager_task_factory(loop, my_coroutine())

though I"m not 100% sure they're totally equivalent.

I'm a tiny bit leaning to not support this right now if we aren't sure, but maybe I'm missing something today.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is also my understanding from reading the documentation.

@drdavella drdavella requested a review from bdoyal February 14, 2024 14:52
@drdavella
Copy link
Member

I've asked @bdoyal to take a look at the test cases just as another sanity check to make sure we're producing correct code.

Copy link
Member

@drdavella drdavella left a comment

Choose a reason for hiding this comment

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

This is a very nice codemod overall. I agree with @andrecsilva that it looks like we need to handle eager tasks slightly differently. Hopefully that's not too difficult. Also I requested some small changes to the metadata.

src/core_codemods/fix_task_instantiation.py Outdated Show resolved Hide resolved
src/core_codemods/fix_task_instantiation.py Outdated Show resolved Hide resolved
class FixTaskInstantiation(SimpleCodemod, NameAndAncestorResolutionMixin):
metadata = Metadata(
name="fix-task-instantiation",
summary="Use high-level `asyncio.create_task` API",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
summary="Use high-level `asyncio.create_task` API",
summary="Use `asyncio.create_task` API to instantiate `asyncio.Task`",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drdavella I don't think we should use your exact summary wording bc the codemod may change to not only asyncio.create_task, but also to some_loop.create_task and now to asyncio.eager_task_factory which is why I just said high-level API.

Copy link
Member

Choose a reason for hiding this comment

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

How about Use high-level asyncio APIs to instantiate tasks?

Also I think our convention is to use title case for the summary.

Copy link
Member

Choose a reason for hiding this comment

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

Nvm you basically did that already. I didn't even notice 🤦

Copy link
Member

@bdoyal bdoyal left a comment

Choose a reason for hiding this comment

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

@drdavella asked me to take a look at the aysnc inputs/outputs in one of the test files, so I'm just here to leave comments on that.

tests/codemods/test_fix_task_instantiation.py Outdated Show resolved Hide resolved
@clavedeluna
Copy link
Contributor Author

latest commits add support for eager_task. this is a new feature in py3.12 and the docs are a bit confusing and there aren't a lot of examples online, so hopefully I got it right.

Copy link
Member

@drdavella drdavella left a comment

Choose a reason for hiding this comment

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

Just asking for one more very minor change but otherwise looks 👍

),
],
)
change_description = "Replace instantiation of `asyncio.Task` higher-level functions to create tasks."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
change_description = "Replace instantiation of `asyncio.Task` higher-level functions to create tasks."
change_description = "Replace instantiation of `asyncio.Task` with higher-level functions to create tasks."

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@clavedeluna clavedeluna added this pull request to the merge queue Feb 16, 2024
Merged via the queue into main with commit 6fa0615 Feb 16, 2024
12 checks passed
@clavedeluna clavedeluna deleted the task-init branch February 16, 2024 19:49
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.

4 participants