Allow split_out to be None, which then defaults to 1 in groupby().aggregate()

[ian-r-rose]

• Closes Can no longer provide split_out=None as a default in groupby.aggregate #9490
• Tests added / passed
• Passes pre-commit run --all-files

[jrbourbeau]

Thanks @ian-r-rose! Would you mind adding a small test (e.g. similar to the example in #9490)?

[jrbourbeau]

Also, should we deprecate allowing None here?

[ian-r-rose]

Thanks @ian-r-rose! Would you mind adding a small test (e.g. similar to the example in #9490)?

I was a bit uncomfortable adding a test since I wasn't sure if we actually wanted to support None here. I'd be curious to hear from @wence- if there are any things that would become hugely onerous if we dropped that support.

[jrbourbeau]

Agreed, it'd be good to get thoughts from @wence- on his particular use case. That said, do we advertise anywhere that split_out can be None? From a brief skim of the API docs, it looks like we're consistent in saying it's an int

[ian-r-rose]

I only see None documented in one place (sort of), which is the docstring for groupby.aggregate: https://docs.dask.org/en/latest/generated/dask.dataframe.groupby.DataFrameGroupBy.aggregate.html#dask.dataframe.groupby.DataFrameGroupBy.aggregate

Even then, it's saying it's an optional arg, which is a bit ambiguous.

[ian-r-rose]

The implementation for apply_concat_apply clearly anticipates a None value, but I might consider that an internal API.

[wence-]

The implementation for apply_concat_apply clearly anticipates a None value, but I might consider that an internal API.

That also has a bug :( (sort=True, split_out=None => TypeError)

[wence-]

Thanks @ian-r-rose! Would you mind adding a small test (e.g. similar to the example in #9490)?

I was a bit uncomfortable adding a test since I wasn't sure if we actually wanted to support None here. I'd be curious to hear from @wence- if there are any things that would become hugely onerous if we dropped that support.

I don't think it would be problematic. I noticed because in dask_cudf we explicitly test groupby().agg with split_out=None. But that is in tests so I can remove it.

The maintenance issue here is somehow that there are lots of public APIs that call into the few "semi-private" APIs. So there are lots of places for the defaults to go out of whack.

One way to partially solve this is to not provide default values for arguments to internal APIs, and promote the default handling to the public callers. I generally think that internal APIs should not do argument defaulting, since all that business logic handling is not really germane to the usage of the function.

It does make calling these functions slightly more verbose (especially given that they all have many arguments), though one can utilise my favourite python argument list approach of required keyword-only args:

def aca(*, split_out, split_every, ...)

[ian-r-rose]

though one can utilise my favourite python argument list approach of required keyword-only args:

If I ever have a week of nothing to do, I'd love to make 90% of the APIs here make use of keyword-only args.

[wence-]

Also, should we deprecate allowing None here?

I am inclined to say yes. I will change things on the dask_cudf side.

[rjzamora]

My concern with dropping support for split_out=None is that we may find a reasonable way to automatically infer a good value for split_out in the future. For example, perhaps the Groupby class itself should optionally infer group cardinality up-front. In that case, we would probably not want to override split_out if it was already set to a specific integer value.

This hypothetical concern may not be realistic.

[ian-r-rose]

My concern with dropping support for split_out=None is that we may find a reasonable way to automatically infer a good value for split_out in the future. For example, perhaps the Groupby class itself should optionally infer group cardinality up-front. In that case, we would probably not want to override split_out if it was already set to a specific integer value.

This hypothetical concern may not be realistic.

I think it's a realistic hypothetical, but I guess that in that case I would advocate for expanding the accepted types to include None, or a sentinel value like the pandas NoDefault when it actually comes up. That change shouldn't be API-breaking for, except to the extent that we might want to make inference default.

[jrbourbeau]

Thanks @ian-r-rose!