[WIP] df.apply: add support for engine='bodo'
#60622
Conversation
|
Is there an issue open to discuss this? Adding another engine internally is a non-trivial amount of work and maintenance, so I'm not sure we would even add this without discussion up front. |
@WillAyd Hi Will, thank you for the feedback, I would be happy to open an issue and start a conversation. The intention of this draft was mainly experimentation. |
WillAyd
added
the
Needs Discussion
There was a problem hiding this comment.
Changes look good to me. I added some comments with ideas, but just minor things.
It'd still be good to have an issue for this (or maybe just update the description of the PR), so everybody understands in detail why this is being proposed, and what use cases this addresses. But in general, this seems good.
pandas/core/apply.py
Outdated
| @@ -870,13 +870,16 @@ def apply(self) -> DataFrame | Series: | |||
| "the 'numba' engine doesn't support using " | |||
| "a string as the callable function" | |||
| ) | |||
| if self.engine == "bodo": | |||
There was a problem hiding this comment.
| if self.engine == "bodo": | |
| elif self.engine == "bodo": |
pandas/core/apply.py
Outdated
| results, res_index = self.apply_series_numba() | ||
| else: |
There was a problem hiding this comment.
| else: | |
| elif self.engine == "else": |
I know what you wrote is consistent with what we have now, where numba is the default for the if, but I think it's clearer to avoid a default (or make python the default). Maybe raise a ValueError if self.engine isn't known?
pandas/core/apply.py
Outdated
| @@ -1089,6 +1098,26 @@ def apply_series_numba(self): | |||
| results = self.apply_with_numba() | |||
| return results, self.result_index | |||
|
|
|||
| def apply_series_bodo(self) -> DataFrame | Series: | |||
| bodo = import_optional_dependency("bodo") | |||
There was a problem hiding this comment.
Not sure if importing bodo is immediate, but maybe better to have the checks that raise the errors first, and only import if those pass?
| @@ -10288,6 +10288,8 @@ def apply( | |||
| <https://numba.pydata.org/numba-doc/dev/reference/numpysupported.html>`_ | |||
| in numba to learn what you can or cannot use in the passed function. | |||
|
|
|||
| TODO: describe bodo | |||
There was a problem hiding this comment.
Adding this comment to the review, so the TODO is not forgotten
| frame = pd.DataFrame( | ||
| {"a": [1, 2, 3], "b": [4, 5, 6], "c": [7.0, 8.0, 9.0]}, | ||
| ) | ||
| f = lambda x: x["c"] |
There was a problem hiding this comment.
Not sure why our linter doesn't complain, but I think it's considered a bad practice to use lambda when assigning to a variable. I'd use def instead.
| @@ -3,6 +3,7 @@ | |||
| name: pandas-dev | |||
| channels: | |||
| - conda-forge | |||
| - bodo.ai | |||
There was a problem hiding this comment.
Do you have plans to package Bodo for conda-forge? Is there a reason not to? I think it'd be better for users and our CI if we could simply use conda-forge.
There was a problem hiding this comment.
Yes, we currently have an in progress PR here: conda-forge/staged-recipes#28648
| @@ -0,0 +1,107 @@ | |||
| import numpy as np | |||
There was a problem hiding this comment.
Rather than creating separate tests for bodo is there a way that we can create a fixture for the three different engines? Structuring the tests that way would be very helpful to ensure result consistency
Adds support for specifying engine="bodo" when executing UDF's on DataFrames via
df.apply.doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.