-
Notifications
You must be signed in to change notification settings - Fork 133
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
Add async support for pipe_family #1223
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to f7c7e04 in 1 minute and 12 seconds
More details
- Looked at
62
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. hamilton/function_modifiers/macros.py:610
- Draft comment:
Typo in docstring: 'parameeter' should be 'parameter'. - Reason this comment was not posted:
Confidence changes required:10%
The comment is about a typo in the docstring, which is a minor issue but should be corrected for clarity.
2. hamilton/function_modifiers/macros.py:1310
- Draft comment:
Consider renamingasync_function
to something more descriptive likeasync_identity_wrapper
for clarity. - Reason this comment was not posted:
Confidence changes required:20%
The code is checking if a function is asynchronous and then creating an async wrapper if needed. This is a good approach, but the naming of the async wrapper function could be improved for clarity.
3. hamilton/node.py:370
- Draft comment:
Consider renamingasync_function
to something more descriptive likeasync_new_callable_wrapper
for clarity. - Reason this comment was not posted:
Confidence changes required:20%
The code is checking if a function is asynchronous and then creating an async wrapper if needed. This is a good approach, but the naming of the async wrapper function could be improved for clarity.
4. hamilton/node.py:378
- Draft comment:
Remove the commented-out line to keep the code clean and maintainable. - Reason this comment was not posted:
Confidence changes required:50%
The functionreassign_inputs
innode.py
has a commented-out line that should be removed to keep the code clean and maintainable.
Workflow ID: wflow_dRTAk95K2Or0YLaJ
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -1302,10 +1302,17 @@ def transform_node( | |||
# We pick a reserved prefix that ovoids clashes with user defined functions / nodes | |||
original_node = node_.copy_with(name=f"{node_.name}.raw") | |||
|
|||
is_async = inspect.iscoroutinefunction(fn) # determine if its async |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic to check if a function is async using inspect.iscoroutinefunction
is already present in multiple places in the codebase. Consider reusing existing implementations to avoid redundancy.
- logic to determine if a function is async (expanders.py)
- logic to determine if a function is async (recursive.py)
- logic to determine if a function is async (base.py)
- logic to determine if a function is async (node.py)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the surface looks good (I don't thikn ellipsis has a particularly useful comment), but definitely will want some tests!
Enables running pipe_input, pipe_output and mutate with asyncio.
f7c7e04
to
ae8841b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Enables running pipe_input, pipe_output and mutate with asyncio. Addresses #1193.
Changes
node.reassign_inputs
.How I tested this
Notes
Follows the pattern used in
subdag
by creating anasync def
placeholder callable.Checklist