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

Unify treatment of Expr and IR nodes in cudf-polars DSL #17016

Merged

Conversation

wence-
Copy link
Contributor

@wence- wence- commented Oct 8, 2024

Description

As part of in-progress multi-GPU work, we will likely want to:

  1. Introduce additional nodes into the IR namespace;
  2. Implement rewrite rules for IR trees to express needed communication patterns;
  3. Write visitors that translate expressions into an appropriate description for whichever multi-GPU approach we end up taking.

It was already straightforward to write generic visitors for Expr nodes, since those uniformly have a .children property for their dependents. In contrast, the IR nodes were more ad-hoc. To solve this, pull out the generic implementation from Expr into an abstract Node class. Now Expr nodes just inherit from this, and IR nodes do so similarly.

Redoing the IR nodes is a little painful because we want to make them hashable, so we have to provide a bunch of custom get_hashable implementations (the schema dict, for example, is not hashable).

With these generic facilities in place, we can now implement traversal and visitor infrastructure. Specifically, we provide:

  • a mechanism for pre-order traversal of an expression DAG, yielding each unique node exactly once. This is useful if one wants to know if an expression contains some particular node;
  • a mechanism for writing recursive visitors and then wrapping a caching scheme around the outside. This is useful for rewrites.

Some example usages are shown in tests.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@wence- wence- requested a review from a team as a code owner October 8, 2024 17:12
@wence- wence- requested review from bdice and charlesbluca October 8, 2024 17:12
@github-actions github-actions bot added Python Affects Python cuDF API. cudf.polars Issues specific to cudf.polars labels Oct 8, 2024
@wence- wence- added improvement Improvement / enhancement to an existing function breaking Breaking change labels Oct 8, 2024
Copy link
Member

@rjzamora rjzamora left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @wence- ! I'll probably end up taking several passes over this. Leaving a few comments after my first pass.

@wence- wence- force-pushed the wence/fea/polars-uniform-nodes branch from 48e99d0 to 9ea84a6 Compare October 9, 2024 11:22
@wence- wence- force-pushed the wence/fea/polars-uniform-nodes branch from 9ea84a6 to 87970fc Compare October 10, 2024 09:54
@GregoryKimball GregoryKimball requested a review from madsbk October 10, 2024 13:33
Copy link
Contributor

@Matt711 Matt711 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 things I noticed. I haven't really been a part of the multi-GPU discussions. Can you explain at a high level how the cudf.polars IR graph will be translated to a dask task graph? I think that's the approach @rjzamora mentioned offline.

Edit: I'm asking for my own understanding and because maybe that might help us identify any other helper functions like resuse_if_unchanged that might help with the effort.

@wence-
Copy link
Contributor Author

wence- commented Oct 11, 2024

Just a small things I noticed. I haven't really been a part of the multi-GPU discussions. Can you explain at a high level how the cudf.polars IR graph will be translated to a dask task graph? I think that's the approach @rjzamora mentioned offline.

Not completely straightforwardly. But broadly, write a visitor that emits the correct task graph for computing a node in the plan, given that the children have already been computed (i.e. we have a task graph for them).

Edit: I'm asking for my own understanding and because maybe that might help us identify any other helper functions like resuse_if_unchanged that might help with the effort.

I think that we should wait for that until we write things and then see if we can generalise the implementation.

Right now I know I need reuse-if-unchanged because it's a useful utility for transforming an IR graph into another one of the same type with some new nodes.

@wence- wence- closed this Oct 11, 2024
@wence- wence- reopened this Oct 11, 2024
@wence-
Copy link
Contributor Author

wence- commented Oct 11, 2024

I will rebase on top of #17014 on monday.

@rjzamora
Copy link
Member

Just a small things I noticed. I haven't really been a part of the multi-GPU discussions. Can you explain at a high level how the cudf.polars IR graph will be translated to a dask task graph? I think that's the approach @rjzamora mentioned offline.

Not completely straightforwardly. But broadly, write a visitor that emits the correct task graph for computing a node in the plan, given that the children have already been computed (i.e. we have a task graph for them).

Yes. We will eventually want to implement a visitor that can traverse the final IR/Node graph to generate a task graph that can be scheduled with distributed/dask-cuda.

Further Notes: In practice, it will probably make sense to start by rewriting the initial IR graph into a "partition-aware" IR graph before we generate the task graph. In other words: We probably want to define and propagate leaf-node partitioning, and define boundaries between node sequences that do or do-not require inter-partition communication. Ideally, this partition-aware representation would not have anything Dask-specific in it.

@Matt711
Copy link
Contributor

Matt711 commented Oct 11, 2024

Not completely straightforwardly. But broadly, write a visitor that emits the correct task graph for computing a node in the plan, given that the children have already been computed (i.e. we have a task graph for them).

Yes. We will eventually want to implement a visitor that can traverse the final IR/Node graph to generate a task graph that can be scheduled with distributed/dask-cuda.

Thanks that's a nice overview.

Further Notes: In practice, it will probably make sense to start by rewriting the initial IR graph into a "partition-aware" IR graph before we generate the task graph. In other words: We probably want to define and propagate leaf-node partitioning,

So this "partition aware" graph will need to carry metadata about the partitions? And as the graph is executed this metadata will be carried upward.

and define boundaries between node sequences that do or do-not require inter-partition communication. Ideally, this partition-aware representation would not have anything Dask-specific in it.

I think this is because some operations can execute on a single partition and others can't. What does "inter-partition communication" look like? Is this where shuffling the data comes into play?

@rjzamora
Copy link
Member

So this "partition aware" graph will need to carry metadata about the partitions? And as the graph is executed this metadata will be carried upward.

There are probably several ways to approach this, so we don't need to commit to anything specific right now. We will probably want to be able to ask a node how many partitions it will produce (i.e. how many output tasks it will define). One way to do this is to attach an npartitions-like property to all IR nodes. Most operations will simply inherit the partitioning of its child(ren), but leaf nodes and reductions will depend on the data/algorithm.

I think this is because some operations can execute on a single partition and others can't. What does "inter-partition communication" look like? Is this where shuffling the data comes into play?

Yes. If I do something like add two columns together to assign a new column, then we don't need any communication between partition-0 and partition-1. However, if we want to perform a groupby aggregation, we usually do need to reduce/shuffle data between distinct partitions. When we use dask for execution, we define the inter-partition communication as edges in the task graph.

@wence- wence- force-pushed the wence/fea/polars-uniform-nodes branch from 87970fc to 74a98f0 Compare October 14, 2024 09:49
@wence-
Copy link
Contributor Author

wence- commented Oct 14, 2024

Rebased onto and fixed conflicts after the expression move.

@wence- wence- force-pushed the wence/fea/polars-uniform-nodes branch from a7d050c to 9a8d59e Compare October 14, 2024 11:34
@wence- wence- requested a review from a team as a code owner October 14, 2024 11:34
@wence- wence- force-pushed the wence/fea/polars-uniform-nodes branch from 9a8d59e to 56e1634 Compare October 14, 2024 11:37
@wence- wence- force-pushed the wence/fea/polars-uniform-nodes branch from 56e1634 to 73019c8 Compare October 14, 2024 11:56
Copy link
Contributor

@Matt711 Matt711 left a comment

Choose a reason for hiding this comment

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

Thanks @wence- for adding examples to overview.md. I had some non-blocking questions.

Copy link
Member

@rjzamora rjzamora left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @wence- !

I have a few minor thoughts/questions, but I generally approve of the change.

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

I'm not exactly sure where this is going next since none of the traversal logic seems to actually be used yet, but I'm sure I'll see it soon :)

@wence- wence- force-pushed the wence/fea/polars-uniform-nodes branch from 6e6d68b to f68d914 Compare October 17, 2024 16:05
This simplifies things a bit and means we don't need to type the
children property everywhere else.
@wence- wence- force-pushed the wence/fea/polars-uniform-nodes branch from f68d914 to 6260ff9 Compare October 17, 2024 16:11
@wence- wence- requested a review from vyasr October 18, 2024 08:01
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Looks great. A couple of threads are still open and it would be good to resolve them one way or another, but I don't need to review again. Thanks!

This simplifies the implementation and removes the need for
type-narrowing. The special method dunder-eq handles objects of any
type.
@wence-
Copy link
Contributor Author

wence- commented Oct 21, 2024

/merge

@rapids-bot rapids-bot bot merged commit 637e320 into rapidsai:branch-24.12 Oct 22, 2024
102 checks passed
@wence- wence- deleted the wence/fea/polars-uniform-nodes branch October 22, 2024 10:25
rapids-bot bot pushed a commit that referenced this pull request Oct 30, 2024
…t filters (#17141)

Previously, we always applied parquet filters by post-filtering. This negates much of the potential gain from having filters available at read time, namely discarding row groups. To fix this, implement, with the new visitor system of #17016, conversion to pylibcudf expressions.

We must distinguish two types of expressions, ones that we can evaluate via `cudf::compute_column`, and the more restricted set of expressions that the parquet reader understands, this is handled by having a state that tracks the usage. The former style will be useful when we implement inequality joins.

While here, extend the support in pylibcudf expressions to handle all supported literal types and expose `compute_column` so we can test the correctness of the broader (non-parquet) implementation.

Authors:
  - Lawrence Mitchell (https://github.com/wence-)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #17141
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change cudf.polars Issues specific to cudf.polars improvement Improvement / enhancement to an existing function Python Affects Python cuDF API.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants