Remove special Index class from the general index class hierarchy

[vyasr]

Renames the old Index class to BaseIndex as the parent for all index types and creates a new Index class that subclasses BaseIndex to provide the expected API for cudf.Index. The critical change is that this class has a custom metaclass to make it look like BaseIndex from an inheritance perspective, and it takes on the __new__ method that allows it to return different Index types depending on the input. This change will allow us to rewrite the hierarchy of different Index types in much simpler, more robust, and more performant ways. In particular, we will no longer have to override __new__ in those classes, we will be able to make use of __init__ rather than the custom _initialize method we are currently using, and we reduce ambiguities with respect to which and how many __new__ and __init__ methods are called.

[vyasr]

This should be a non-breaking change, but should be reviewed carefully for edge cases where this solution fails. It will also need to be rebased once the forward merge of 21.06 brings the changes from #8254 onto 21.08, since this relies on that PR.

[shwina]

Note that users who subclass cudf.Index will run into some nasty behaviour:

In [10]: class MyIndex(cudf.Index):
    ...:     pass
    ...:

In [11]: isinstance(cudf.StringIndex(), MyIndex)
True  # !!!

What they should do is subclass cudf.BaseIndex instead. We should probably call this out loudly in our docs for Index.

[shwina]

This should be a non-breaking change

I think the above makes this a breaking change, as any existing user code that subclasses cudf.Index will need to. be changed.

[vyasr]

I think that's a valid concern. I have two main reasons why I think this is OK to do and shouldn't prevent us from moving forward with this PR.

First, someone who subclassed pd.Index in their code and then try to use cudf.Index as a drop-in replacement would already see errors because our internal structures are different. Subclassing pd.Index is already a bit of a minefield: it's not documented on the official pandas documentation of Subclassing pandas data structures, and there are lots of "internal" details that have to be handled by subclasses as documented in this open issue. For those reasons, I don't think promising compatibility with pandas at the level of subclassing Index is something we should even attempt because it requires more than API promises, it requires our internal implementation details to conform to pandas internals as well.

Second, subclassing Index (or any of its subclasses) in its current form is already very difficult, and we don't currently make any promises regarding how to do this (or even document these requirements). At minimum users need to:

1. Override __new__ rather than __init__.
2. Call the correct parent's __new__ method, which for now is equivalent to object.__new__ but is also not documented and is subject to change if we modify any of the parent __new__ methods for any reason.
3. Call _initialize if extending a subclass of Index (e.g. Int64Index) rather than Index itself.
4. Avoid putting any logic in __init__ for performance reasons (because it would be called twice), and absolutely avoid putting any logic in __init__ that is not idempotent because then the second call will fail.

For those reasons, I think we would be justified in just telling users that instead of subclassing Index they should subclass BaseIndex, which will ultimately provide an easier path than pandas does to extending the class.

[shwina]

Understood. Let's add a note along these lines in the cudf.Index docs:

Do not subclass cudf.Index. If you absolutely must, and if you're intimately familiar with the internals of cuDF, subclass cudf.BaseIndex instead.

If nothing, it will serve as a reminder to internal developers...

[vyasr]

If you like, I think that we could explicitly prohibit this with an assert cls is Index in Index.__new__. That would prevent Index.__new__ being called by any subclasses.

[shwina]

I'd be +1 for that

[vyasr]

I'd be +1 for that

Done. Of course this won't save us from subclasses that actually override __new__, but if someone knows enough to override __new__ rather than __init__ and still chooses to move forward I'm fine saying they're on their own.

[shwina]

rerun tests

[vyasr]

This is blocked by rapidsai/integration#282, which is necessary to run 21.08 tests. Once that's merged tests can be rerun.

[vyasr]

rerun tests

[vyasr]

Integration PR is now merged, but this PR is now blocked by #8419 .

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@90e29d9). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 0f8bbb6 differs from pull request most recent head 6e53634. Consider uploading reports for the commit 6e53634 to get more accurate results

@@               Coverage Diff               @@
##             branch-21.08    #8309   +/-   ##
===============================================
  Coverage                ?   82.85%           
===============================================
  Files                   ?      109           
  Lines                   ?    17920           
  Branches                ?        0           
===============================================
  Hits                    ?    14847           
  Misses                  ?     3073           
  Partials                ?        0           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90e29d9...6e53634. Read the comment docs.

[shwina]

LGTM

[quasiben]

Thanks @vyasr . If @rjzamora has time it would be good to get his sign off as well though I think the changes are good

[rjzamora]

I only looked at the minor dask_cudf changes and the index.py changes. This seems fine to me :)

[vyasr]

@gpucibot merge