-
Notifications
You must be signed in to change notification settings - Fork 163
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
[FEAT] Enable init args for stateful UDFs #2956
Conversation
CodSpeed Performance ReportMerging #2956 will not alter performanceComparing Summary
|
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.
LGTM. We should also ensure the other configurations are passed along (I believe the only other one we need to think about is .with_batch_size()
)
tests/expressions/test_udf.py
Outdated
set_planning_config( | ||
config=get_context().daft_planning_config.with_config_values(enable_actor_pool_projections=request.param) | ||
) | ||
yield request.param |
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.
We should remember to reset the config too to the old one after the yield point.
We do this for the execution context (see: daft.context.py
)
@contextlib.contextmanager
def execution_config_ctx(**kwargs):
"""Context manager that wraps set_execution_config to reset the config to its original setting afternwards"""
original_config = get_context().daft_execution_config
try:
set_execution_config(**kwargs)
yield
finally:
set_execution_config(config=original_config)
We can probably just make one for the planning config too!
@jaychia the batch size is passed in already in the rust side that actually calls the stateful UDF: https://github.com/Eventual-Inc/Daft/blob/kevin/stateful-actor-init-args/src/daft-dsl/src/functions/python/udf.rs#L197 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2956 +/- ##
==========================================
+ Coverage 76.34% 78.49% +2.14%
==========================================
Files 597 597
Lines 71388 70091 -1297
==========================================
+ Hits 54504 55017 +513
+ Misses 16884 15074 -1810
Flags with carried forward coverage won't be shown. Click here to find out more.
|
No description provided.