Move GroupBy to Submodule and Add FutureWarning
State: closed

ref #20485 @jreback 

Labels:
name: Refactor
description: Internal refactoring of code
name: Groupby

Comments:
There was a failure in one of the "Allowed Failures" as a result of a `ResourceWarning` for an open TextIOBuffer (?). I don't believe that is related to this change and it didn't appear in any of the previous CI runs
> There was a failure in one of the "Allowed Failures" as a result of a ResourceWarning for an open TextIOBuffer (?). I don't believe that is related to this change and it didn't appear in any of the previous CI runs

FYI there are NO allowed failures, meaning that yes these will allow the build to complete earlier, but generally we don't merge things if they are causing issues.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20506?src=pr&el=h1) Report
> Merging [#20506](https://codecov.io/gh/pandas-dev/pandas/pull/20506?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/4efb39f01f5880122fa38d91e12d217ef70fad9e?src=pr&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/20506/graphs/tree.svg?width=650&src=pr&token=eZ4WkYLtcO&height=150)](https://codecov.io/gh/pandas-dev/pandas/pull/20506?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #20506      +/-   ##
==========================================
+ Coverage   91.82%   91.84%   +0.02%     
==========================================
  Files         152      153       +1     
  Lines       49255    49256       +1     
==========================================
+ Hits        45226    45239      +13     
+ Misses       4029     4017      -12
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.23% <100%> (+0.02%)` | :arrow_up: |
| #single | `41.9% <66.66%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/20506?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/groupby/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/20506/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9ncm91cGJ5LnB5) | `92.55% <ø> (ø)` | |
| [pandas/core/reshape/pivot.py](https://codecov.io/gh/pandas-dev/pandas/pull/20506/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9waXZvdC5weQ==) | `96.97% <100%> (ø)` | :arrow_up: |
| [pandas/core/resample.py](https://codecov.io/gh/pandas-dev/pandas/pull/20506/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzYW1wbGUucHk=) | `96.43% <100%> (ø)` | :arrow_up: |
| [pandas/core/api.py](https://codecov.io/gh/pandas-dev/pandas/pull/20506/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXBpLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [pandas/core/groupby/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/20506/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9fX2luaXRfXy5weQ==) | `100% <100%> (ø)` | |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/20506/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `95.94% <100%> (ø)` | :arrow_up: |
| [pandas/core/panel.py](https://codecov.io/gh/pandas-dev/pandas/pull/20506/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcGFuZWwucHk=) | `97.29% <100%> (ø)` | :arrow_up: |
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/20506/diff?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `66.81% <0%> (+1.73%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20506?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20506?src=pr&el=footer). Last update [4efb39f...7624313](https://codecov.io/gh/pandas-dev/pandas/pull/20506?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

FYI, merge conflict with master.
Hmm so I'm not sure that the FutureWarning being in the `__init__.py` of the groupby package is ideal here because the internal references point to `pandas.core.groupby.groupby`. I think warning in that init will always get thrown, because the new module is part of the package where that `__init__.py` exists, so it would always get loaded?

`pandas.core.categorical` didn't have this problem because the updated reference was higher in the package hierarchy. The deprecated module doesn't get loaded when using the newer reference. Thinking through what our options here are but open to ideas
Can we just import what is needed in `__init__.py`? Then no deprecation warning is needed (python doesn't care if it is a single file module or a package directory, so we can make use of that).
Personally I would even also use those imports internally. Then you don't need to change all the imports in the rest of pandas, if you reorganize things internally within the ``groupby`` subpackage.
@jorisvandenbossche I already updated all the internal refs per Jeff's previous comment, but could undo if required. That said, maybe we just shouldn't put the warning on the __init__ given the change could be transparent from an API perspective since all we are doing is moving from a single module to a like-named package?
> That said, maybe we just shouldn't put the warning on the init given the change could be transparent from an API perspective since all we are doing is moving from a single module to a like-named package?

Yes, that's exactly what I tried to say :-) 

IMO, this is much cleaner (also internally, but certainly for external packages)
very tiny style issue, ping on green.
@jreback did you see our discussion above?

So -1 on merge on green. There is no need to deprecate this (and we can still move to a subpackage)
@jorisvandenbossche sorry, I did see that. yeah we can not deprecate is fine.
Does anyone develop on Windows? Trying to figure out why CircleCI is failing but can't reproduce the issue locally. 

An easy way to solve it would be to add BinGrouper back to the API in the `__init__.py` but that doesn't seem required for anything else internal, so figure it's best to figure out what is different about the Windows import mechanism here
CircleCI is also linux, not windows. But why it is failing there and not in the others, I don't know. 
But you can always import `from pandas. .... import BinGrouper` and then test against that, that should certainly work.

I would also like to advocate to keep the internal imports the same as before (so import into `__init__.py` everything that is "public" for the rest of pandas)
@jreback what do you think about Joris' comment? Should I put everything back into the `__init__.py` and undo the changes to internal references or simply update this last failing test case?
i would move non public this out of the __init__
thanks @WillAyd