-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
[Data] Make name
a required argument for AggregateFn
#44880
Conversation
Signed-off-by: Scott Lee <sjl@anyscale.com>
accumulate_row: Callable[[AggType, T], AggType] = None, | ||
accumulate_block: Callable[[AggType, Block], AggType] = None, | ||
finalize: Callable[[AggType], U] = lambda a: a, | ||
name: Optional[str] = None, |
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.
Did AggregateFn
always error if name
isn't specified? If there are cases where AggregateFn
worked without a name, this'd be a breaking change.
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.
all the existing constructor usages in ray specify a name. i think the error occurs when we call aggregate
on a GroupedDataset
without providing a name.
if there are external users using this without a name, it could potentially break, but i think throwing a TypeError
specific for this input should suffice.
accumulate_row: Callable[[AggType, T], AggType] = None, | ||
accumulate_block: Callable[[AggType, Block], AggType] = None, | ||
finalize: Callable[[AggType], U] = lambda a: a, | ||
name: Optional[str] = None, |
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.
Should also add a check. Changing the type hint doesn't have any effect at runtime.
Signed-off-by: Scott Lee <sjl@anyscale.com>
Signed-off-by: Scott Lee <sjl@anyscale.com>
Why are these changes needed?
Currently,
name
is an optional arg forAggregateFn
, which can cause errors when the user defines a customAggregateFn
without a name. For example, here we will get an empty row due to theAggregateFn.name
not being set.This PR makes the
name
parameter required forAggregateFn
, which resolves the issue.Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.