API/ERR: allow iterators in df.set_index & improve errors

[h-vetinari]

• closes the parts of API: better error-handling for df.set_index #22484 (resp. those worth keeping) that were reverted in Revert set_index inspection/error handling for 0.24.1 #25085 due to Regression in DataFrame.set_index with class instance column keys #24969 closes Regression in DataFrame.set_index with class instance column keys #24969
• tests added / passed
• passes git diff upstream/master -u -- "*.py" | flake8 --diff

This is a quick fix for the regression - however, I think this should be immediately (i.e. 0.24.1) be deprecated. I haven't yet added a deprecation warning here, pending further discussion in the issue.

@jorisvandenbossche @TomAugspurger @jreback

[h-vetinari]

If supporting custom types, we need to go back to the state pre-#22486 that just puts everything that's not listlike in list. Otherwise we can't guarantee that iteration below will work.

[jreback]

I think that is best, let's just revert entirely to pre 22486

[h-vetinari]

pre-#22486 had some other problems we do not need to re-instate (e.g. weird KeyErrors if it's an iter).

I agree that the code complexity must stay maintainable, and so the most reasonable thing (IMO), ist to just not bother trying to "fix" unhashable custom types being used as keys. At that point, the user is so far gone off the reservation that it's really not our job to give them a good error message (again, even the repr of such a frame would crash).

[wkschwartz]

Can str(col) normally raise a KeyError?

[h-vetinari]

The error might be different, that's true. On second thought, str defaults to repr if it's not implemented, so unless the class manages to break its own repr (haha), this should not be necessary.

[codecov]

Codecov Report

Merging #24984 into master will decrease coverage by <.01%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master   #24984      +/-   ##
==========================================
- Coverage   92.38%   92.38%   -0.01%     
==========================================
  Files         166      166              
  Lines       52400    52405       +5     
==========================================
+ Hits        48409    48413       +4     
- Misses       3991     3992       +1

Flag	Coverage Δ
#multiple	90.8% <90%> (-0.01%)	⬇️
#single	42.88% <10%> (-0.01%)	⬇️

Impacted Files	Coverage Δ
pandas/core/frame.py	96.88% <90%> (-0.05%)	⬇️

---

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 bf693ff...3e01681. Read the comment docs.

[codecov]

Codecov Report

Merging #24984 into master will decrease coverage by <.01%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master   #24984      +/-   ##
==========================================
- Coverage   91.73%   91.73%   -0.01%     
==========================================
  Files         173      173              
  Lines       52856    52877      +21     
==========================================
+ Hits        48490    48509      +19     
- Misses       4366     4368       +2

Flag	Coverage Δ
#multiple	90.3% <90.47%> (ø)	⬆️
#single	41.69% <52.38%> (ø)	⬆️

Impacted Files	Coverage Δ
pandas/compat/__init__.py	58.03% <50%> (-0.07%)	⬇️
pandas/core/frame.py	96.85% <94.73%> (-0.03%)	⬇️

---

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 3855a27...5f99b15. Read the comment docs.

[TomAugspurger]

Thanks for the PR.

I'm not sure what should be done going forward, or whether we should have a deprecation now. @toobaz may have thoughts here too. I'm fine with leaving that for a comprehensive "what's allowed an Index" overhaul.

[wkschwartz]

Reposting #24969 (comment) here in case the discussion of the deprecation of custom label types proceeds here instead of at the original ticket.

Obviously I would prefer no deprecation of custom label types as they are integral to my company’s applications. However, I would urge strongly that if you do decide to deprecate the feature, you do so starting only in the next major release (presumably 0.25.0) rather than in a minor release (0.24.1).

If you do stop supporting custom label types and I am not to be stuck at Pandas 0.23.4 forever, I could theoretically undertake the (expensive) refactoring to use unique IDs (my production code has the equivalent of the name field from my toy example in the OP). However, other users whose code would break might not have convenient unique IDs to switch to. Please do not remove this feature lightly.

[toobaz]

@toobaz may have thoughts here too

I'm boring, I always have the same thoughts, every time the discussion comes out :-) And they are:

• if we didn't say it's not allowed, it's allowed, and should work
• if there is no reason to disallow it, let's not say it's not allowed
• if we think that the reason to disallow it is that it makes code simpler, we are almost always wrong. Clean code will work with any type a user throws in, as long as it satisfies general properties (e.g. __hash__). Viceversa, explicitly listing the types we "like" as scalar will require us to amend the list (and the docs) every time we find some new type that yes, we think is worth allowing.
• (a valid reason to disallow it is that either it brings ambiguity, or it changes the semantics, as would be accepting lists as keys)

[toobaz]

Don't get me wrong: it is good to have this discussion once and for all if it results in cleaner docs/assumptions. But my opinion is that the docs should say "you can use as keys anything which is not mutable".

[wkschwartz]

But my opinion is that the docs should say "you can use as keys anything which is not mutable".

"Not mutable" is a bit fuzzy in Python. If, instead, keys are required to be hashable (which usually implies some sort of recursive immutability), then users already familiar with Python dicts would immediately get it, you can test for key-eligibility with isinstance(potential_key, collections.abc.Hashable) or try: hash(potential_key) \n except TypeError: ..., and you'll maintain backward compatibility. Moreover, you'll always be able to convert a DataFrame to/from dicts, which most folks already assume they can do freely (at least as far as I've seen).

The documentation should then also have a sentence or two about the importance of immutability and compatibility between the object's __hash__ and __eq__ implementations. A mention of ints, strings, tuples, namedtuples, and dataclass's frozen parameter would then give users some ideas.

In the Python 3.7.2 session below, a is mutable, b is technically immutable, but not sufficiently so for its hash to work (suggesting that isinstance(potential_key, collections.abc.Hashable) isn't quite good enough).

>>> a = []
>>> b = (a,)
>>> hash(b)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unhashable type: 'list'

[toobaz]

"Not mutable" is a bit fuzzy in Python. If, instead, keys are required to be hashable

Yep, my mistake, "hashability" is the right property.

[h-vetinari]

I'm not a core dev, but FWIW, here are some counter points to @toobaz arguments:

if there is no reason to disallow it, let's not say it's not allowed

If the code is not designed and tested for it, such a carte blanche is (not in this example, but across 1000s of small cuts) an immense burden down the road when something that wasn't explicitly forbidden suddenly breaks.

if we didn't say it's not allowed, it's allowed, and should work

See lack of testing above. Duck typing really bites us in the ass here. Stability (a possible SemVer after 1.0) even more.

if we think that the reason to disallow it is that it makes code simpler, we are almost always wrong. Clean code will work with any type a user throws in, as long as it satisfies general properties (e.g. __hash__).

The previous behaviour was specifically causing some weird KeyErrors for stuff that should have never been tested as a key (hence I'm adding a try-except in this PR), see #22484. I could have easily maintained this capability, if it had been tested, or even documented.

Viceversa, explicitly listing the types we "like" as scalar will require us to amend the list (and the docs) every time we find some new type that yes, we think is worth allowing.

That - in contrast to the above - would make it a controlled (non-breaking) expansion where necessary (IMO far preferable). I'm also thinking the ecosystem has matured enough by now to broadly have a common understanding what "scalar" means.

(a valid reason to disallow it is that either it brings ambiguity, or it changes the semantics, as would be accepting lists as keys)

That line can be blurred arbitrarily far (see my example in the issue).

Don't get me wrong: it is good to have this discussion once and for all if it results in cleaner docs/assumptions. But my opinion is that the docs should say "you can use as keys anything which is not mutable".

In #24702, it sounded like you're advocating against using tuples as keys (which I would agree with), which also aren't mutable are hashable

IMO, the API should have a clearly defined surface, not an arbitrary "whatever happens to work". The latter would mean (and already does to some extent) a disproportionate amount of dev time to chase down weird corner cases, rather than focus on a consistent API that's useful and unambiguous for 99.99% of the cases.

[toobaz]

If the code is not designed and tested for it

You test lines of code, not user behavior. Otherwise tests suites would have to be infinite. You write code that satisfies properties, not just that doesn't break your tests. Tests are a very useful addition, but you will never test any possible combination of parameters, even if you list one by one the possible values they can take. Otherwise, since duck typing is the norm in Python, you would be accusing a very large number of projects of not being able to write effective tests.

Notice, by the way, that "custom types" is not a precise definition. People might feed our indices with types that they did not create, but other libraries did.

The previous behaviour was specifically causing some weird KeyErrors for stuff that should have never been tested as a key (hence I'm adding a try-except in this PR), see #22484.

Feel free to expand, I'm not following you. Notice that Index subclasses have all the right to raise TypeErrors, because there the types are clearly defined.

In #24702, it sounded like you're advocating against using tuples as keys

Not at all. I said multiple times in many other discussions that tuples (with hashable content) are perfectly valid keys. I actually used exactly the same arguments I'm using here, that's why I'm boring :-)

IMO, the API should have a clearly defined surface, not an arbitrary "whatever happens to work"

That's sure. I'm being very clear (although @wkschwartz beat me at it) on what the API should support. And it should work on what it supports.

That line can be blurred arbitrarily far (see my example in the issue).

Not following you. I'm replying there.

[h-vetinari]

You test lines of code, not user behavior. Otherwise tests suites would have to be infinite.

Not if there's a well-defined set of input types. ;-)

Feel free to expand, I'm not following you.

The iterator that gets passed to df.set_index (see OP of #22484), say, in the expectation of its values being used as the index, somehow gets turned into an Index and then raises a KeyError. That's one of several confusing examples.

That's sure. I'm being very clear (although @wkschwartz beat me at it) on what the API should support. And it should work on what it supports.

Ok, that's a more nuanced discussion then (vs. "everything that works is allowed"). If all hashable objects should be usable as keys, we'd have to drastically improve our testing for it, but it's not impossible to try to support it, certainly. I just think it's far too wide a line to draw. I mentioned in the issue that it would be more feasible to add dataclasses specifically to the types we test, rather than all hashables (and to be sure: my point is not about @wkschwartz use case - much less against him).

[toobaz]

If all hashable objects should be usable as keys, we'd have to drastically improve our testing for it, but it's not impossible

Again, non sequitur. Testing is good, but we want to write code that works even in those cases in which it was not tested.

Not if there's a well-defined set of input types. ;-)

OK, let's cut it short: will you soon be asking us to limit the values of Series/DataFrames to well-defined set of input types? I hope not. Do you think this is a problem for our testing suite? I hope not.

If you need to test any possible combination of allowed input types with every possible behavior of every method in the pandas API, tests will be just ''slightly short'' of infinite.

(see OP of #22484)

I see three points there, and not sure if any of them is a problem. Can you assume I'm even more stupid than I am?

General comment: I still wasn't able to understand if this regression was a mistake or was made on purpose. In the second case, while I can only blame myself for not being able to keep up with the PRs, I still think such a change deserved some more discussion, e.g. in mailing list.

[wkschwartz]

To expand on @toobaz's #24984 (comment): in a duck-typed language, APIs should be based on protocols, interfaces, or capabilities, not on exact class hierarchies. For example, Python core does fine defining the API surface of dict keys as anything hashable.

Regarding whether that which is not forbidden is allowed (TWINFIA): Pandas' having a version < 1 means the maintainers have the right to change semantics. Please balance that right against the mass of established code bases in production that rely on Pandas. I couldn't be the only author to assume TWINFIA.

Thank you, @h-vetinari and @toobaz, for taking an interest in this!

[h-vetinari]

Again, non sequitur. Testing is good, but we want to write code that works even in those cases in which it was not tested.

I agree that properly written code, that only ever depends on hashability for keys, is possible. But many methods (not just set_index) need to do a lot of gynmastics for wildly varying input signatures - in this case, column keys, list of column keys, various arrays, lists of arrays, and mixes between arrays and keys.
This to me is a large reason of the utility of pandas, because very different (but equally sensible) scenarios just work. Allowing arbitrary types for keys makes this "magic" much more complicated, but OK, opinions can differ what should be the design goal.

Not if there's a well-defined set of input types. ;-)

OK, let's cut it short: [strawman]

Let me clarify my language here: it's certainly possible to test methods against various container types that it should accept. What's in those containers is another story.

(see OP of #22484)

I see three points there, and not sure if any of them is a problem. Can you assume I'm even more stupid than I am?

I know you're not (stupid), but ok, here's another try:

>>> import pandas as pd
>>> import numpy as np
>>>
>>> df = pd.DataFrame(np.random.randn(5, 5), columns=list('ABCDE'))
>>>
>>> # Series-constructor uses iterators as if it was an array
>>> my_iter = map(str, df.A)
>>> pd.Series(my_iter)
0     0.8812414814169731
1    -1.0789948340108944
2    -0.2049936946205116
3    0.35952380472779194
4     -0.529524992759882
>>>
>>> # so why not do the same for df.set_index, which *also* works with arrays?
>>> my_iter = map(str, df.A)  # had been consumed previously
>>> df.set_index(my_iter)
Traceback (most recent call last):
[...]
KeyError: "None of [Index(['0.8812414814169731', '-1.0789948340108944', '-0.2049936946205116',\n       '0.35952380472779194', '-0.529524992759882'],\n      dtype='object')] are in the [columns]"

There's a couple things that are confusing here. How did an iterator get turned into an Index object, without any further user-input? And why is that Index being tested as a column key?!

General comment: I still wasn't able to understand if this regression was a mistake or was made on purpose.

There was no informed decision on this regression because it was neither tested nor documented (if the documented standard had been "all hashable objects can be key", it would have been easy to conform to that). The problems were much more mundane, i.e. having to inspect the items of the outer-most container and determine whether they are keys / Index / Series / MultiIndex / np.ndarray. Through several rounds of review @jreback then added the review requirment to add objects that pass is_list_like (which got reverted just before the release) but that would have still broken this use case here.

The common ground here is (as we've seen above) to have a well-defined API. If what can be keys is explicitly documented somewhere, it can be kept in mind for whatever code that's being written, and hashability would be one of several valid choices for that.

My main point above was that enforcing code to stay runnable that only works because of an oversight (and there's a bunch of that in pandas, no matter how much we strive for clean code) is way too generous.

[toobaz]

But many methods (not just set_index) need to do a lot of gynmastics for wildly varying input signatures

Claim: identifying lists (more in general, containers) requres gymnastics (and, incidentally, detailed docs), while identifying valid keys is extremely simple if their definition is "hashable object". And requires more complicated code, more complicated docs, and more waste of the user's memory, if the definition is more complicated than that.

As in: ExtensionArray (and having even our own types rely on them) is already making (I think) the pandas codebase cleaner, and easier to maintain, precisely because we gave up setting the admissible values.

I know you're not (stupid), but ok, here's another try:

Thanks, I appreciate. You are perfectly right that pandas' definition of "list-like" is a mess, and that's a ground where we desperately need to set standards. Or maybe, we already all agree on standards (iterators are list-like, tuples are not), and there is just annoying legacy code that doesn't follow them (for sure is_list_like doesn't).

I think that's not the case for keys, where things are... simple.

EDIT: sure, list-likes and keys definitions are not so orthogonal as I am putting them. I'm clearly assuming that if something is a list-like, then it is not a key, by definition. So if the user has hashable list-likes, the user will need to know that they are not valid keys (as they are valid containers). But "outlawing" types which are not list-likes is orthogonal.

[TomAugspurger]

Steering the discussion back to the PR: what needs to be done for 0.24.1? IMO, the changes and tests look good, just need a release note under "fixed regressions". Anything else?

[jorisvandenbossche]

Looks good to me, just a minor comment, and the whatsnew note that Tom asked for.

[jorisvandenbossche]

I think doing col in self.columns is a bit more efficient? (doesn't need construct the series)
Although then you also need to care about the False case

[h-vetinari]

I tried, but col in self.columns works without the hashing, and even if check

hash(col) and col in self.columns

this does not catch the iter case (an iterator is hashable, who would have thought...)

[jorisvandenbossche]

Why is the hashing needed? We just need to know whether it is a column name or not?
Or you want to raise a different error message?

[jorisvandenbossche]

And it certainly checks hashing for some cases:

In [5]: {1} in pd.Index([1, 2, 3])
...
TypeError: unhashable type: 'set'

(just not an iter because as you said, it is hashable)

[h-vetinari]

@jreback

[h-vetinari]

@jreback PTAL

[jreback]

use this issue number, what about the note for the closed issue in the PR itself? #24969

[jreback]

does this still close #24969 ?

[h-vetinari]

I updated the OP to clarify, and added the this PR as you asked

[h-vetinari]

OK, I now see that #24969 is still open, but that was already closed by #25085

[h-vetinari]

Thanks for the review, PTAL

[h-vetinari]

I updated the OP to clarify, and added the this PR as you asked

[h-vetinari]

One of the azure jobs failed with a ResourceWarning (they're back? yay!)

[...]
s...........sys:1: ResourceWarning: unclosed file <_io.TextIOWrapper name=0 mode='r' encoding='UTF-8'>

=================================== FAILURES ===================================
______________ test_chunks_have_consistent_numerical_type[python] ______________
[gw0] linux -- Python 3.6.8 /home/vsts/miniconda3/envs/pandas-dev/bin/python

all_parsers = <pandas.tests.io.parser.conftest.PythonParser object at 0x7f37938b8b00>

    def test_chunks_have_consistent_numerical_type(all_parsers):
        parser = all_parsers
        integers = [str(i) for i in range(499999)]
        data = "a\n" + "\n".join(integers + ["1.0", "2.0"] + integers)
    
        # Coercions should work without warnings.
        with tm.assert_produces_warning(None):
>           result = parser.read_csv(StringIO(data))

pandas/tests/io/parser/test_common.py:1078: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <contextlib._GeneratorContextManager object at 0x7f37472facc0>
type = None, value = None, traceback = None

    def __exit__(self, type, value, traceback):
        if type is None:
            try:
>               next(self.gen)
E               AssertionError: Caused unexpected warning(s): [('ResourceWarning', ResourceWarning('unclosed <ssl.SSLSocket [closed] fd=18, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6>',), '/home/vsts/work/1/s/pandas/io/parsers.py', 2854)].

Could someone please restart it? @jreback @TomAugspurger @jorisvandenbossche @gfyoung

[h-vetinari]

Thanks to whoever restarted the job. Unfortunately, it seems to be a non-transient break (which is good, at least from the point of hunting ResourceWarnings, but bad for what else I should be accomplishing today).

My operating assumption is that a new boto release broke something, but unfortunately I can't see the version in the build script. The conda list call comes after the environment is installed.

[h-vetinari]

Added the conda list call in #25377 and merged it in here.

[h-vetinari]

Nevermind, this is passing again. Seems those ResourceWarnings will have to wait for another day.

[jorisvandenbossche]

Since it is passing again, can you then remove the conda list changes here?

[h-vetinari]

@jorisvandenbossche
Feel free to revert the last commit yourself, otherwise I'll do it tonight (Europe).

[jreback]

yeah pls revert all of this

[h-vetinari]

@jreback @jorisvandenbossche
This should hopefully be done now.

[jreback]

this is just a code reorg yes? with slightly better error messages in some cases?

[h-vetinari]

@jreback:
Minimal reorg, just better errors and enabling iterators (due to your review).

The custom classes were re-enabled by #25085 (which took over the tests from this PR), which closed the regression #24969, and has a corresponding whatsnew note. I guess the issue didn't get closed yet, because I only noted that #25085 was an alternative to this PR (at the time, for solving #24969), but didn't add the "closes #24969" explicitly - sorry.

[h-vetinari]

@jreback
Can we push this one over the finish line? (new commit was just due to whatsnew conflict)