justin-yan (CONTRIBUTOR) commented on 2021-02-24 20:52:24:
Got it - that's super helpful, I was trying to figure out what would break!

I think there are two issues we're discussing here:

1. modifying the pa_type of ClassLabel: totally agree with you on that one if that's OK from a back-compat perspective.  (i.e. are users of `datasets` not supposed to access or use the .pa_type attribute of ClassLabel?)
2. creating a ClassLabel requires information that's not present on the pa.DictionaryType object:  I think the crux of the problem is that at this line (https://github.com/huggingface/datasets/pull/1936/files#diff-54081ede051fd0a7ef65748c481cc06f90209f01bb89968747089d13a2ca052bR933) - you only have access to the `pa_type`, which is `DictionaryType[int8, string]`. I've unpacked it and looked at all of the available methods, and I don't believe that any of the actual values ("names") are present - those are stored on the `pyarrow.DictArray.dictionary` attribute (i.e. as data, not on the pyarrow.DataType) - so in order to actually be able to instantiate the ClassLabel with the names= parameter, we need to pass in more information to this method.

We *could* mostly accomplish this by modifying https://github.com/huggingface/datasets/pull/1936/files#diff-54081ede051fd0a7ef65748c481cc06f90209f01bb89968747089d13a2ca052bR909 to accept a pyarrow Table in addition to the type, and it's not too difficult to do, but it feels a little bit off to me:

- It feels a bit off that a "schema" definition will change depending on what data gets added to the dataset.  In particular, if someone adds rows or concatenates two datasets, the ClassLabel "names" will also need to change, right?  I think maybe we're getting around this because a Dataset is immutable (I think?) and so any new dataset is freshly constructed, but for example - I think this check wouldn't work for `ClassLabel`s if we were to compare the `Dataset.features` instead of the underlying pyarrow type https://github.com/huggingface/datasets/blob/master/src/datasets/arrow_dataset.py#L2664
- To that end I wonder if ClassLabel should actually just be the "type" akin to Category, and the "names" should be considered "data" and not part of the "type"? Similar to how pyarrow maintains two data objects - the array of indices and the array of string values.

With that in mind, I'm wondering if you *should* allow an empty ClassLabel (and`int2str`, etc. can be updated to have more descriptive error messages if labels aren't provided or inferred), and if the underlying data is a pa.DictionaryType, then the names can be inferred and applied at these points in the code:
- https://github.com/huggingface/datasets/blob/96578adface7e4bc1f3e8bafbac920d72ca1ca60/src/datasets/arrow_dataset.py#L274
- https://github.com/huggingface/datasets/blob/96578adface7e4bc1f3e8bafbac920d72ca1ca60/src/datasets/arrow_dataset.py#L686
- https://github.com/huggingface/datasets/blob/96578adface7e4bc1f3e8bafbac920d72ca1ca60/src/datasets/arrow_dataset.py#L673

I think perhaps the mismatch here is when the data is stored on disk as an int there should be a convenient way of saying "this is a dictionary and here are some explicitly provided labels", whereas when it's stored as a string, we'd ideally like to say "this is a Category and please condense the representation and automatically infer the labels".

Sorry for the long comment!  Hopefully my thoughts make sense - thanks for taking the time to discuss!