-
Notifications
You must be signed in to change notification settings - Fork 903
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
Separate evaluation logic from IR
objects in cudf-polars
#17175
Separate evaluation logic from IR
objects in cudf-polars
#17175
Conversation
We will use this to provide infrastructure for making IR nodes easier to traverse. Expr nodes already use this facility, but we want to share it.
And tests of basic functionality.
This way we will be able to write generic traversals more easily.
Now that we have a uniform child attribute, this is easier.
This simplifies things a bit and means we don't need to type the children property everywhere else.
…into rjzamora/refactor-evaluate
5efd67a
to
65fe592
Compare
# Hacky to avoid type-checking issues, just advertise the | ||
# signature. Both mypy and pyright complain if we have an abstract | ||
# method that takes arbitrary *args, but the subclasses have | ||
# tighter signatures. This complaint is correct because the | ||
# subclass is not Liskov-substitutable for the superclass. | ||
# However, we know do_evaluate will only be called with the | ||
# correct arguments by "construction". | ||
do_evaluate: Callable[..., DataFrame] |
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.
Not sure if I like this any better than just using a catch-all signature. For example, mypy seems happy with:
@classmethod
def do_evaluate(cls, *args: Any, **kwargs: Any):
"""
Evaluate the node (given its evaluated children), and return a dataframe.
Parameters
----------
args
Non child arguments followed by any evaluated dataframe inputs.
kwargs
Key-word arguments. Should be empty!
Returns
-------
DataFrame (on device) representing the evaluation of this plan
node.
Raises
------
NotImplementedError
If we couldn't evaluate things. Ideally this should not occur,
since the translation phase should pick up things that we
cannot handle.
"""
raise NotImplementedError(
f"Evaluation of plan {type(cls).__name__}"
) # pragma: no cover
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.
If you remove kwargs from the signature (since there are none), it complains though
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.
Yes, I agree it's still "off" :)
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.
It looks like we need to account for _non_child_args
elements that are normalized within __init__
. (Left a few suggestions, but didn't look for everything yet)
@wence- - Just a note (still thinking on this): I've been experimenting with the next steps (beyond this PR). I'm realizing that it may not be terribly useful to separate the IR evaluation logic unless we also do the same for Expr classes. I say this because there seem to be many expressions that require reductions and/or shuffles. |
Can we do that in a separate step? I would like to check the utility of this setup in the "single-partition" evaluation model first. |
Yes, sorry. I'm not suggesting that we do anything further in this PR (I feel that this is ready for a final review). The goal of my previous comment was to establish that we may need to do do something similar to |
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.
Minor feedback. Generally looks good to me.
/merge |
Description
Closes #17127
IR.evaluate
convention.Checklist