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

- [x] closes the parts of #22484 (resp. those worth keeping) that were reverted in #25085 due to #24969 ~closes #24969~
- [x] tests added / passed
- [x] 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 


Labels:
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24984?src=pr&el=h1) Report
> Merging [#24984](https://codecov.io/gh/pandas-dev/pandas/pull/24984?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/bf693ffb520da3b315737a8d52595646afa74d63?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `90%`.

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

```diff
@@            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%)` | :arrow_down: |
| #single | `42.88% <10%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24984?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/24984/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.88% <90%> (-0.05%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24984?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/24984?src=pr&el=footer). Last update [bf693ff...3e01681](https://codecov.io/gh/pandas-dev/pandas/pull/24984?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24984?src=pr&el=h1) Report
> Merging [#24984](https://codecov.io/gh/pandas-dev/pandas/pull/24984?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/3855a27be4f04d15e7ba7aee12f0220c93148d3d?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `90.47%`.

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

```diff
@@            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%> (ø)` | :arrow_up: |
| #single | `41.69% <52.38%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24984?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/compat/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/24984/diff?src=pr&el=tree#diff-cGFuZGFzL2NvbXBhdC9fX2luaXRfXy5weQ==) | `58.03% <50%> (-0.07%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/24984/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.85% <94.73%> (-0.03%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24984?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/24984?src=pr&el=footer). Last update [3855a27...5f99b15](https://codecov.io/gh/pandas-dev/pandas/pull/24984?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Reposting https://github.com/pandas-dev/pandas/issues/24969#issuecomment-458246305 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 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)
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".
>  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).
```python-console
>>> a = []
>>> b = (a,)
>>> hash(b)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unhashable type: 'list'
```
> "Not mutable" is a bit fuzzy in Python. If, instead, keys are required to be _hashable_

Yep, my mistake, "hashability" is the right property.
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](https://github.com/pandas-dev/pandas/issues/24969#issuecomment-458241015) 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](https://docs.python.org/3/glossary.html#term-immutable)

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.
> 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 ``TypeError``s, 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](https://github.com/pandas-dev/pandas/issues/24969#issuecomment-458241015) in the issue).

Not following you. I'm replying there.
> 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).
> 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``/``DataFrame``s 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.
To expand on @toobaz's https://github.com/pandas-dev/pandas/pull/24984#issuecomment-458342687: 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](https://en.wikipedia.org/wiki/Everything_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!
> 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.
> 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](https://github.com/pandas-dev/pandas/issues/24702)).

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.
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?
@h-vetinari did you keep a test for a TypeError being raised when given unhashable key? (I don't see it in the diff, but I assume it was already there?)
> I disagree with that quite strongly, again based on the iter example. The intention of that is clearly different, and should not raise a KeyError.

@h-vetinari I agree that the `map` example that you give above (https://github.com/pandas-dev/pandas/pull/24984#issuecomment-458426437) is indeed confusing, and it would be nice to solve this (or keep it solved). 
But, you are making this a special case (raising a ValueError instead of KeyError) for one specific example (`iter`), while there may be other cases that will follow this path that are not necessarily confusing like that (eg a custom class, but simply a missing key). So, alternatively, I think we can also solve this with a better error message, instead of another error: we can also make the generic KeyError for a missing key into a more informative error message. Something like (in italic my addition to the existing error):

The parameter "keys" may be a column key, one-dimensional array, or a list containing only valid column keys and one-dimensional arrays. *The passed object {} of type {} is regarded as a column key, but not found.*

I think that would actually also be an improvement for the case that you eg pass a list instead of an array (and meant it to be new values instead of a column key).

> @jorisvandenbossche: @h-vetinari did you keep a test for a TypeError being raised when given unhashable key? (I don't see it in the diff, but I assume it was already there?)

I was being tested through `set`, but raises a `ValueError` there. This was the reason I added `hash(col)` originally, because `test_set_index_raise_on_type` trying `df.set_index({1, 2, 3})` did not raise a `TypeError` but a `KeyError`.

Your suggestion of adding the types for the missing keys is not bad (though it would still be better to raise something else than `KeyError` for that), but a bit complicated to implement, especially if there are several keys (and the code is called from different places).

In any case, I think it's cleaner to keep raising a `ValueError` for the iterator/generator cases, and use the `KeyError` otherwise. I've implemented that in the last commit and also reflected it in the tests. I also added a missing key test for the custom type for you.
@jorisvandenbossche 
Alright, this should now hopefully cover all bases, *even* hashable listlikes. I'm only raising `TypeError` for those hashable listlikes that have no length (like iterators/generators), and leaving the rest pass through to being tested as keys.
I would rather revert entirely to 0.23.4 and have these changes sit in master.  

I am just really wary of changing this and ensure correctness. The prior PR regressed and now this is partially reverting / changing. I think it would be better to completely revert (though leave the tests). Then put this change to sit in master for 0.25.0
What are the pertinent differences between the 0.23.4 implementation and this implementation?

> On Feb 1, 2019, at 06:57, Jeff Reback <notifications@github.com> wrote:
> 
> I would rather revert entirely to 0.23.4 and have these changes sit in master.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

I also would prefer to just reverting to pre 0.23.4 behavior for the next release. IIUC a lot of the churn here started with #22486 which improved error messaging in some fringe cases. 

My point I was trying to make on Gitter yesterday is I feel like we are cramming in a lot of churn to the code base for edge cases; would rather slow down and target a later release to look back at these comprehensively than keep patching on a tight timeline
Note that this PR is *mostly* doing that, reverting to 0.23.4 behaviour. 
We could also actually 'git revert' #22486, but I am not sure that is needed (it also included some welcome code clean-up that is not related to those behavioural changes, additional tests, ..).

But it is true that we could revert more. Eg specifically revert the full "validation of the passed values" block that was added in #22486 (while keeping the code clean-up and additional tests). 
Personally I think it would be nice to keep the better error *messages* (not necessarily the error *type*)


> @TomAugspurger: What are the pertinent differences between the 0.23.4 implementation and this implementation?

Comparing to 0.23.4 / pre-#22486, the whole inspection part below is new (which does nothing but raise better errors, rather than just plonking whatever into `frame[col]`)

https://github.com/pandas-dev/pandas/blob/794f61d4d77d075b37e40144ed3adbda669ace64/pandas/core/frame.py#L4149-L4181

The part where the values are then collected has been cleaned up, but basically only very minor changes.
https://github.com/pandas-dev/pandas/blob/794f61d4d77d075b37e40144ed3adbda669ace64/pandas/core/frame.py#L4198-L4216

Compare with (last [commit](https://github.com/pandas-dev/pandas/commits/0.23.x) of 23.4):
https://github.com/pandas-dev/pandas/blob/0409521665bd436a10aea7e06336066bf07ff057/pandas/core/frame.py#L3889-L3913

Using `set` in the following was added (part of solving #22484):
https://github.com/pandas-dev/pandas/blob/794f61d4d77d075b37e40144ed3adbda669ace64/pandas/core/frame.py#L4225-L4227

Can you take a look at that Will? Any concerns?

> On Feb 1, 2019, at 10:57, h-vetinari <notifications@github.com> wrote:
> 
> @TomAugspurger: What are the pertinent differences between the 0.23.4 implementation and this implementation?
> 
> Comparing to 0.23.4 / pre-#22486, the whole inspection part below is new (which does nothing but raise better errors, rather than just plonking whatever into frame[col])
> 
> https://github.com/pandas-dev/pandas/blob/794f61d4d77d075b37e40144ed3adbda669ace64/pandas/core/frame.py#L4149-L4181
> 
> The part where the values are then collected has been cleaned up, but basically only very minor changes.
> https://github.com/pandas-dev/pandas/blob/794f61d4d77d075b37e40144ed3adbda669ace64/pandas/core/frame.py#L4198-L4216
> 
> Compare with (last commit of 23.4):
> https://github.com/pandas-dev/pandas/blob/0409521665bd436a10aea7e06336066bf07ff057/pandas/core/frame.py#L3889-L3913
> 
> Using set in the following was added (part of solving #22484):
> https://github.com/pandas-dev/pandas/blob/794f61d4d77d075b37e40144ed3adbda669ace64/pandas/core/frame.py#L4225-L4227
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

> @WillAyd: IIUC a lot of the churn here started with #22486 which improved error messaging in some fringe cases.

That custom types got excluded was indeed an oversight (cf. lack of testing), but the churn had to do with the orthogonal review requirement to allow all list-likes (against my goals for the PR, and in spite of warnings about the ambiguity of the tuple case). I pointed this out again in other discussions #22225 / #24046, which lead to removing those list-likes again with #24762.

IOW, I don't quite agree that the improved errors were "fringe", and I will reject blame for the fact that these PRs caused churn. Rather, I was proactively trying to rectify/improve the situation before a release - also with this PR.

OTOH, I agree with you about not supporting these custom cases, and would have preferred to deprecate them already for 0.24.1. However, the prevailing consensus is to fix the regression first, so I tried to achieve that, and to provide another (IMO better) option than just reverting.
@h-vetinari want to be clear not blaming you or that PR for anything - this kind of stuff just happens :-)
I still think Jeff's approach on how to move forward is the best in this situation. OK with whatever you ultimately decide though
> @jorisvandenbossche: Note that this PR is *mostly* doing that, reverting to 0.23.4 behaviour.

For some reason, your post hadn't popped up in my tab before I wrote the detailed code comparison, but you're right. After the back-and-forth with list-likes, the only thing that is now changed compared to 0.23.4 is better errors and more tests. 

> We could also actually 'git revert' #22486 [...]

Not that I don't understand the reluctance to keep adding more PRs on top...

In order to do a full revert, one would have to also (and first) revert #24762. If that's the consensus, this could be done on the 0.24.x-branch without touching master, I'm guessing? Naturally, I would prefer the error improvements here though.
@WillAyd do you have a chance to take a look through 
https://github.com/pandas-dev/pandas/pull/24984#issuecomment-459791046?

and here's a diff of the `set_index` implementation prior to #22486 and this PR

<details>

```diff
diff --git a/a b/b
index 5db9299..ed97d58 100644
--- a/a
+++ b/b
@@ -1,7 +1,46 @@
         inplace = validate_bool_kwarg(inplace, 'inplace')
+
+        err_msg = ('The parameter "keys" may be a column key, one-dimensional '
+                   'array, or a list containing only valid column keys and '
+                   'one-dimensional arrays.')
+
         if not isinstance(keys, list):
             keys = [keys]
 
+        missing = []
+        for col in keys:
+            if (is_scalar(col) or isinstance(col, tuple)):
+                # if col is a valid column key, everything is fine
+                # tuples are always considered keys, never as list-likes
+                if col not in self:
+                    missing.append(col)
+            elif isinstance(col, (ABCIndexClass, ABCSeries,
+                                  np.ndarray, list)):
+                # arrays are fine as long as they are one-dimensional
+                if getattr(col, 'ndim', 1) > 1:
+                    raise ValueError(err_msg)
+            elif is_list_like(col) and not hasattr(col, '__len__'):
+                # various iterators/generators are hashable, but should not
+                # raise a KeyError; we identify them by their lack of __len__.
+                # hashable listlikes with __len__ get tested as keys below.
+                tipo = type(col)
+                raise TypeError(err_msg + ' Received column of '
+                                'type {}'.format(tipo))
+            else:
+                # everything else gets tried as a key; see GH 24969
+                try:
+                    found = col in self.columns
+                except TypeError:
+                    tipo = type(col)
+                    raise TypeError(err_msg + ' Received column of '
+                                    'type {}'.format(tipo))
+                else:
+                    if not found:
+                        missing.append(col)
+
+        if missing:
+            raise KeyError('{}'.format(missing))
+
         if inplace:
             frame = self
         else:
@@ -11,7 +50,7 @@
         names = []
         if append:
             names = [x for x in self.index.names]
-            if isinstance(self.index, MultiIndex):
+            if isinstance(self.index, ABCMultiIndex):
                 for i in range(self.index.nlevels):
                     arrays.append(self.index._get_level_values(i))
             else:
@@ -19,29 +58,23 @@
 
         to_remove = []
         for col in keys:
-            if isinstance(col, MultiIndex):
-                # append all but the last column so we don't have to modify
-                # the end of this loop
-                for n in range(col.nlevels - 1):
+            if isinstance(col, ABCMultiIndex):
+                for n in range(col.nlevels):
                     arrays.append(col._get_level_values(n))
-
-                level = col._get_level_values(col.nlevels - 1)
                 names.extend(col.names)
-            elif isinstance(col, Series):
-                level = col._values
-                names.append(col.name)
-            elif isinstance(col, Index):
-                level = col
+            elif isinstance(col, (ABCIndexClass, ABCSeries)):
+                # if Index then not MultiIndex (treated above)
+                arrays.append(col)
                 names.append(col.name)
-            elif isinstance(col, (list, np.ndarray, Index)):
-                level = col
+            elif isinstance(col, (list, np.ndarray)):
+                arrays.append(col)
                 names.append(None)
+            # from here, col can only be a column label
             else:
-                level = frame[col]._values
+                arrays.append(frame[col]._values)
                 names.append(col)
                 if drop:
                     to_remove.append(col)
-            arrays.append(level)
 
         index = ensure_index_from_sequences(arrays, names)
 
@@ -50,7 +83,8 @@
             raise ValueError('Index has duplicate keys: {dup}'.format(
                 dup=duplicates))
 
-        for c in to_remove:
+        # use set to handle duplicate column names gracefully in case of drop
+        for c in set(to_remove):
             del frame[c]
 
         # clear up memory usage
```

</details>

And as @h-vetinari says, a simple revert of #22486 may very-well turn up dependent issues.