SergeiGKS/huggingface-dataset-issues huggingface dataset:
Issue: Incorrect filepath for dill module

Comment:
I'm using spyder, and I am still getting `0.3.6` for `dill`, so unfortunately #288 isn't applicable, I think.  However, I found something odd that I believe is a clue: 

```
import inspect
import dill

inspect.getfile(dill)
>>> '/Users/avivbrokman/opt/anaconda3/lib/python3.9/site-packages/dill/__init__.py'
```

I checked out the directory, and there is no `dill` subdirectory within '/Users/avivbrokman/opt/anaconda3/lib/python3.9/site-packages/dill`, as there should be.  Rather, `_dill.py` is in  '/Users/avivbrokman/opt/anaconda3/lib/python3.9/site-packages/dill` itself. 

 If I run `pip install dill` or `pip install --upgrade dill`, I get the message `Requirement already satisfied: dill in ./opt/anaconda3/lib/python3.9/site-packages (0.3.6)`.  If I run `conda upgrade dill`, I get the message `Solving environment: failed with repodata from current_repodata.json, will retry with next repodata source.` a couple of times, followed by

```
Solving environment: failed
Solving environment: / 
Found conflicts! Looking for incompatible packages.
```

And then terminal proceeds to list conflicts between different packages I have.

This is all very strange to me because I recently uninstalled and reinstalled `anaconda`.
Issue: Incorrect filepath for dill module

Comment:
As I said above, I guess this is not a problem with `datasets`. I think you have different Python environments: one with the new dill version (the one you get while using pip) and other with the old dill version (the one where you get the AttributeError).

You should update `dill` in the Python environment you are using within spyder.

Please note that the `_dill` module is present in the `dill` package since their 2.8.0 version.
Issue: Adding `sep` to TextConfig 

Comment:
Hi @omar-araboghli, thanks for your proposal.

Have you tried to use "csv" loader instead of "text"? That already has a `sep` argument.
Issue: Adding `sep` to TextConfig 

Comment:
Hi @albertvillanova, thanks for the quick response!

Indeed, I have been trying to use `csv` instead of `text`. However I am still not able to define range of rows as one sequence, that is achievable with passing `sample_by='paragraph'` to the `TextConfig`

For instance, the below code

```python
import datasets

dataset = datasets.load_dataset(
    path='csv',
    data_files={'train': TRAINING_SET_PATH},
    sep='\t',
    header=None,
    column_names=['tokens', 'pos_tags', 'chunk_tags', 'ner_tags']
)
```

leads to 

```python
dataset
>>> DatasetDict({
    train: Dataset({
        features: ['tokens', 'pos_tags', 'chunk_tags', 'ner_tags'],
        num_rows: 62543
    })
})

dataset['train'][0]
>>> {'tokens': 'Distribution',
 'pos_tags': 'NN',
 'chunk_tags': 'O',
 'ner_tags': 'O'
}
```
Is there a way to deal with multiple csv rows as one dataset instance, where each column is a sequence of those rows ?
Issue: Reload features from Parquet metadata

Comment:
I'd be happy to have a look, if nobody else has started working on this yet @lhoestq. 

It seems to me that for the `arrow` format features are currently attached as metadata [in `datasets.arrow_writer`](https://github.com/huggingface/datasets/blob/5f810b7011a8a4ab077a1847c024d2d9e267b065/src/datasets/arrow_writer.py#L412)  and retrieved from the metadata at `load_dataset` time using [`datasets.features.features.from_arrow_schema`](https://github.com/huggingface/datasets/blob/5f810b7011a8a4ab077a1847c024d2d9e267b065/src/datasets/features/features.py#L1602). 

This will need to be replicated for `parquet` via calls to [this api](https://arrow.apache.org/docs/python/generated/pyarrow.parquet.write_metadata.html) from `io.parquet.ParquetWriter` and `io.parquet.ParquetReader` [respectively](https://github.com/huggingface/datasets/blob/5f810b7011a8a4ab077a1847c024d2d9e267b065/src/datasets/io/parquet.py#L104).

Any other important considerations?
Issue: Reload features from Parquet metadata

Comment:
Thanks @MFreidank ! That's correct :)

Reading the metadata to infer the features can be ideally done in the `parquet.py` file in `packaged_builder` when a parquet file is read. You can cast the arrow table to the schema you get from the features.arrow_schema
Issue: Load a cached dataset as iterable

Comment:
Hi ! Sure :) you can comment `#self-assign` to assign yourself to this issue.

I can give you some pointers to get started:

`load_dataset` works roughly this way:
1. it instantiate a dataset builder using `load_dataset_builder()`
2. the builder download and prepare the dataset as Arrow files in the cache using `download_and_prepare()`
3. the builder returns a Dataset object with `as_dataset()`

One way to approach this would be to implement `as_iterable_dataset()` in `builder.py`.

And similarly to `as_dataset()`, you can use the `ArrowReader`. It has a `get_file_instructions()` method that can be helpful. It gives you the files to read as list of dictionaries with those keys: `filename`, `skip` and `take`.

The `skip` and `take` arguments are used in case the user wants to load a subset of the dataset, e.g.
```python
load_dataset(..., split="train[:10]")
```

Let me know if you have questions or if I can help :)
Issue: Load a cached dataset as iterable

Comment:
This use-case is a bit specific, and `load_dataset` already has enough parameters (plus, `streaming=True` also returns an iterable dataset, so we would have to explain the difference), so I think it would be better to add `IterableDataset.from_file` to the API (more flexible and aligned with the goal from https://github.com/huggingface/datasets/issues/3444) instead.
Issue: Load a cached dataset as iterable

Comment:
> This use-case is a bit specific

This allows to use `datasets` for large scale training where map-style datasets are too slow and use too much memory in PyTorch. So I would still consider adding it.

Alternatively we could add this feature one level bellow:
```python
builder = load_dataset_builder(...)
builder.download_and_prepare()
ids = builder.as_iterable_dataset()
```
Issue: Load a cached dataset as iterable

Comment:
Yes, I see how this can be useful. Still, I think `Dataset.to_iterable` + `IterableDataset.from_file` would be much cleaner in terms of the API design (and more flexible since `load_dataset` can only access the "initial" (unprocessed) version of a dataset).

And since it can be tricky to manually find the "initial" version of a dataset in the cache, maybe `load_dataset` could return an iterable dataset streamed from the cache if `streaming=True` and the cache is up-to-date.
Issue: Load a cached dataset as iterable

Comment:
> This allows to use datasets for large scale training where map-style datasets are too slow and use too much memory in PyTorch.

I second that. e.g. In my last experiment Oscar-en uses 16GB RSS RAM per process and when using multiple processes the host quickly runs out cpu memory.
Issue: Load a cached dataset as iterable

Comment:
>And since it can be tricky to manually find the "initial" version of a dataset in the cache, maybe load_dataset could return an iterable dataset streamed from the cache if streaming=True and the cache is up-to-date.

This is exactly the need on JeanZay (HPC) - I have the dataset cache ready, but the compute node is offline, so making streaming work off a local cache would address that need.

If you will have a working POC I can be the tester.
Issue: Load a cached dataset as iterable

Comment:
> Yes, I see how this can be useful. Still, I think Dataset.to_iterable + IterableDataset.from_file would be much cleaner in terms of the API design (and more flexible since load_dataset can only access the "initial" (unprocessed) version of a dataset).

I like `IterableDataset.from_file` as well. On the other hand `Dataset.to_iterable` first requires to load a Dataset object, which can take time depending on your hardware and your dataset size (sometimes 1h+).

> And since it can be tricky to manually find the "initial" version of a dataset in the cache, maybe load_dataset could return an iterable dataset streamed from the cache if streaming=True and the cache is up-to-date.

That would definitely do the job. I was suggesting a different parameter just to make explicit the difference between
- streaming from the raw data
- streaming from the local cache

But I'd be fine with streaming from cache is the cache is up-to-date since it's always faster. We could log a message as usual to make it explicit that the cache is used
Issue: Load a cached dataset as iterable

Comment:
> I was suggesting a different parameter just to make explicit the difference between

MosaicML's `streaming` library does the same (tries to stream from the local cache if possible), so logging a message should be explicit enough :).
Issue: Load a cached dataset as iterable

Comment:
Hi Both! It has been a while since my first issue so I am gonna go for this one ! #self-assign
Issue: Load a cached dataset as iterable

Comment:
https://github.com/huggingface/datasets/pull/5821 should be helpful to implement `IterableDataset.from_file`, since it defines a new ArrowExamplesIterable that takes an Arrow tables generator function (e.g. from a file) and can be used in an IterableDataset
Issue: Load a cached dataset as iterable

Comment:
So what's recommanded usage of `IterableDataset.from_file` and `load_dataset`? How about I have multiple arrow files and `load_dataset` is often convenient to handle that.
Issue: Load a cached dataset as iterable

Comment:
If you have multiple Arrow files you can load them using

```python
from datasets import load_dataset

data_files = {"train": ["path/to/0.arrow", "path/to/1.arrow", ..., "path/to/n.arrow"]}

ds = load_dataset("arrow", data_files=data_files, streaming=True)
```

This is equivalent to calling `IterableDataset.from_file` and `concatenate_datasets`.
Issue: Unpin sqlalchemy once issue is fixed

Comment:
@albertvillanova It looks like that issue has been fixed so I made a PR to unpin sqlalchemy!
Issue: Unpin sqlalchemy once issue is fixed

Comment:
The source issue:
- https://github.com/pandas-dev/pandas/issues/40686

has been fixed:
- https://github.com/pandas-dev/pandas/pull/48576

The fix was released yesterday (2023-04-03) only in `pandas-2.0.0`:
- https://github.com/pandas-dev/pandas/releases/tag/v2.0.0

but it will not be back-ported to `pandas-1`:
- https://github.com/pandas-dev/pandas/pull/48576#issuecomment-1466467159

Also note that `pandas-2.0.0` dropped support for Python 3.7:
- https://github.com/pandas-dev/pandas/issues/41678
- https://github.com/pandas-dev/pandas/pull/41989

Therefore, we cannot unpin `sqlalchemy` until we drop support for Python 3.7 (these Python users cannot use `pandas-2`).
Issue: Dataset scan time is much slower than using native arrow

Comment:
Hi ! In your code you only iterate on the Arrow buffers - you don't actually load the data as python objects. For a fair comparison, you can modify your code using:
```diff
-     for _ in range(0, len(table), bsz):
-         _ = {k:table[k][_ : _ + bsz] for k in cols}
+     for _ in range(0, len(table), bsz):
+         _ = {k:table[k][_ : _ + bsz].to_pylist() for k in cols}
```

I re-ran your code and got a speed ratio of 1.00x and 1.02x
Issue: Dataset scan time is much slower than using native arrow

Comment:
Ah I see, datasets is implicitly making this conversion. Thanks for pointing that out!

If it's not too much, I would also suggest updating some of your docs with the same `.to_pylist()` conversion in the code snippet that follows [here](https://huggingface.co/course/chapter5/4?fw=pt#:~:text=let%E2%80%99s%20run%20a%20little%20speed%20test%20by%20iterating%20over%20all%20the%20elements%20in%20the%20PubMed%20Abstracts%20dataset%3A).
Issue: Dataset scan time is much slower than using native arrow

Comment:
This code snippet shows `datasets` code that reads the Arrow data as python objects already, there is no need to add to_pylist. Or were you thinking about something else ?
Issue: Column project operation on `datasets.Dataset`

Comment:
Hi ! This would be a nice addition indeed :) This sounds like a duplicate of https://github.com/huggingface/datasets/issues/5468

> Not sure. Some of my PRs are still open and some do not have any discussions.

Sorry to hear that, feel free to ping me on those PRs
Issue: Allow opposite of remove_columns on Dataset and DatasetDict

Comment:
Hi! I agree it would be nice to have a method like that. Instead of `keep_columns`, we can name it `select_columns` to be more aligned with PyArrow's naming convention (`pa.Table.select`).
Issue: Allow opposite of remove_columns on Dataset and DatasetDict

Comment:
Hi, I am a newbie to open source and would like to contribute. @mariosasko can I take up this issue ?
Issue: Allow opposite of remove_columns on Dataset and DatasetDict

Comment:
Hey, I also want to work on this issue I am a newbie to open source.
Issue: Allow opposite of remove_columns on Dataset and DatasetDict

Comment:
This sounds related to https://github.com/huggingface/datasets/issues/5474

I'm fine with `select_columns`, or we could also override `select` to also accept a list of columns maybe ?
Issue: Allow opposite of remove_columns on Dataset and DatasetDict

Comment:
@lhoestq, I am planning to add a member function to the dataset class to perform the selection operation. Do you think its the right way to proceed? or there is a better option ?
Issue: Allow opposite of remove_columns on Dataset and DatasetDict

Comment:
Unless @mariosasko thinks otherwise, I think it can go in `Dataset.select()` :)
Though some parameters like keep_in_memory, indices_cache_file_name or writer_batch_size wouldn't when selecting columns, so we would need to update the docstring as well
Issue: Allow opposite of remove_columns on Dataset and DatasetDict

Comment:
If someone wants to give it a shot, feel free to comment `#self-assign` and it will assign the issue to you.

Feel free to ping us here if you have questions or if we can help :)
Issue: Allow opposite of remove_columns on Dataset and DatasetDict

Comment:
I would rather have this functionality as a separate method. IMO it's always better to be explicit than to have an API where a single method can do different/uncorrelated things (somewhat reminds me of Pandas, and there is probably a good reason why PyArrow is more rigid in this aspect).
Issue: Allow opposite of remove_columns on Dataset and DatasetDict

Comment:
In the end I also think it would be nice to have it as a separate method, this way we can also have it for `IterableDataset` (which can't have `select` for indices)
Issue: NonMatchingChecksumError for hendrycks_test

Comment:
Thanks for reporting, @sarahwie.

Please note this issue was already fixed in `datasets` 2.6.0 version:
- #5040

If you update your `datasets` version, you will be able to load the dataset:
```
pip install -U datasets
```
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
Ccing @dwofk (the author of `fast-depth`). 

Thanks, @awsaf49 for reporting this. I believe this is because the NYU Depth V2 shipped from `fast-depth` is already preprocessed. 

If you think it might be better to have the NYU Depth V2 dataset from BTS [here](https://huggingface.co/datasets/sayakpaul/nyu_depth_v2) feel free to open a PR, I am happy to provide guidance :)
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
Good catch ! Ideally it would be nice to have the datasets in the raw form, this way users can choose whatever processing they want to apply
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
> Ccing @dwofk (the author of `fast-depth`).
> 
> Thanks, @awsaf49 for reporting this. I believe this is because the NYU Depth V2 shipped from `fast-depth` is already preprocessed.
> 
> If you think it might be better to have the NYU Depth V2 dataset from BTS [here](https://huggingface.co/datasets/sayakpaul/nyu_depth_v2) feel free to open a PR, I am happy to provide guidance :)

@sayakpaul I would love to create a PR on this. As this will be my first PR here, some guidance would be helpful.

Need a bit of advice on the dataset, there are three publicly available datasets. Which one should I consider for PR?
1. [BTS](https://github.com/cleinc/bts): Containst train/test: 36K/654 data, dtype = `uint16` hence more precise
2. [DenseDepth](https://github.com/ialhashim/DenseDepth) It contains train/test: 50K/654 data, dtype = `uint8` hence less precise
3. [Official](https://cs.nyu.edu/~silberman/datasets/nyu_depth_v2.html#raw_parts):  Size is big 400GB+, requires **MatLab** code for fixing **projection** and **sync**, DataType: `pgm` and `dump` hence can't be used directly.

cc: @lhoestq
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
I think BTS. Repositories like https://github.com/vinvino02/GLPDepth usually use BTS. Also, just for clarity, the PR will be to https://huggingface.co/datasets/sayakpaul/nyu_depth_v2. Once we have worked it out, we can update the following things:

* https://github.com/huggingface/blog/pull/718
* https://huggingface.co/docs/datasets/main/en/depth_estimation

Don't worry about it if it seems overwhelming. We will work it out together :) 

@lhoestq what do you think?
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
@sayakpaul If I get this right I have to,
1. Create a PR on https://huggingface.co/datasets/sayakpaul/nyu_depth_v2
2. Create a PR on https://github.com/huggingface/blog
3. Create a PR on https://github.com/huggingface/datasets to update https://github.com/huggingface/datasets/blob/main/docs/source/depth_estimation.mdx
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
Yup opening a PR to use BTS on https://huggingface.co/datasets/sayakpaul/nyu_depth_v2 sounds good :) Thanks for the help !
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
Finally, I have found the origin of the **discretized depth map**. When I first loaded the datasets from HF I noticed it was 30GB but in DenseDepth data is only 4GB with dtype=uint8. This means data from fast-depth (before loading to HF) must have high precision. So when I tried to dig deeper by directly loading depth_map from `h5py`, I found depth_map from `h5py`  came with `float32`. But when the data is processed in HF with `datasets.Image()` it was directly converted to `uint8` from `float32`  hence the **discretized** depth map.
https://github.com/huggingface/datasets/blob/c78559cacbb0ca6e0bc8bfc313cc0359f8c23ead/src/datasets/features/image.py#L91-L93

## Solutions:

#### 1. Array2D
Use `Array2D` feature with `float32` for depth_map 

* Code:
```py
Features({'depth_map': Array2D(shape=(480, 640), dtype='float32')})
```
* Pros:
No precision loss.

* Cons:
As depth_map is saved as Array I think it can't be visuzlied in [hf.co/dataset](https://huggingface.co/datasets/sayakpaul/nyu_depth_v2) page like segmentation mask.

#### 2. Uint16
Use `uint16` as dtype for Image in `_h5_loader` for saving depth maps and accept `uint16` dtype in `datasets.Image()` feature.

* Code
```py
depth = np.array(h5f["depth"])
depth /= 10.0 # [0, max_depth] -> [0, 1]
depth *= (2**16 -1) # transform from [0, 1] -> [0, 2^16 - 1]
depth = depth.astype('uint16')
```
* Pros:
    * We can visualize depth map in hf.co/datasets page like segmentation mask.
    * No need for post-processing.

* Cons:
     * We need to make two change
          * Modify `_h5_loader` in https://huggingface.co/datasets/sayakpaul/nyu_depth_v2 to convert depth_map from `float32` to `uint16`.
          * Make sure `datasets.Image()` converts `np.ndarray` to `uint16` checking max value
     * Precision loss due to `float32` to `uint16`
     * Post-processing required for depth_map to transform from `[0, 2^16 - 1]` to `[0, max_depth]`  before feeding them to model.
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
Thanks so much for digging into this. 

Since the second solution entails changes to core datatypes in `datasets`, I think it's better to go with the first solution. 

@lhoestq WDYT?
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
@sayakpaul Yes, Solution 1 requires minimal change and provides no precision loss. But I think support for `uint16` image would be a great addition as many datasets come with `uint16` image. For example [UW-Madison GI Tract Image Segmentation](https://www.kaggle.com/competitions/uw-madison-gi-tract-image-segmentation) dataset, here the image itself comes with `uint16` dtype rather than mask. So, saving `uint16` image with `uint8` will result in precision loss.

Perhaps we can adapt solution 1 for this issue and Add support for `uint16` image separately?
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
Using Array2D makes it not practical to use to train a model - in `transformers` we expect an image type.

There is a pull request to support more precision than uint8 in Image() here: https://github.com/huggingface/datasets/pull/5365/files

we can probably merge it today and do a release right away
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
Fantastic, @lhoestq! 

@awsaf49 then let's wait for the PR to get merged and then take the next steps?
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
The PR adds support for uint16 which is ok for BTS if I understand correctly, would it be ok for you ?
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
If the main issue with the current version of NYU we have on the Hub is related to the precision loss stemming from `Image()`, I'd prefer if `Image()` supported float32 as well.
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
I also prefer `float32` as it offers more precision. But I'm not sure if we'll be able to visualize image with `float32` precision.
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
We could have a separate loading for the float32 one using Array2D, but I feel like it's less convenient to use due to the amount of disk space and because it's not an Image() type. That's why I think uint16 is a better solution for users
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
Yep. I just tested from the PR and it works:
```python
>>> train_dataset = load_dataset("sayakpaul/nyu_depth_v2", split="train", streaming=True)                                                         
Downloading readme: 100%|██████████████████| 8.71k/8.71k [00:00<00:00, 3.60MB/s]
>>> next(iter(train_dataset))
{'image': <PIL.PngImagePlugin.PngImageFile image mode=RGB size=640x480 at 0x1382ED7F0>,
 'depth_map': <PIL.TiffImagePlugin.TiffImageFile image mode=F size=640x480 at 0x1382EDF28>}
>>> x = next(iter(train_dataset))
>>> np.asarray(x["depth_map"])                                             
array([[0.       , 0.       , 0.       , ..., 0.       , 0.       ,
        0.       ],
       [0.       , 0.       , 0.       , ..., 0.       , 0.       ,
        0.       ],
       [0.       , 0.       , 0.       , ..., 0.       , 0.       ,
        0.       ],
       ...,
       [0.       , 2.2861192, 2.2861192, ..., 2.234162 , 2.234162 ,
        0.       ],
       [0.       , 2.2861192, 2.2861192, ..., 2.234162 , 2.234162 ,
        0.       ],
       [0.       , 2.2861192, 2.2861192, ..., 2.234162 , 2.234162 ,
        0.       ]], dtype=float32)
```
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
Great! the case is closed! This issue has been solved and I have to say, it was quite the thrill ride. I felt like Sherlock Holmes, solving a mystery and finding the bug🕵️‍♂️. But in all seriousness, it was a pleasure working on this issue and I'm glad we could get to the bottom of it.

On another note, should I consider closing the issue? I think we still need to make updates on https://github.com/huggingface/blog and https://github.com/huggingface/datasets/blob/main/docs/source/depth_estimation.mdx
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
Haha thanks Mr Holmes :p

maybe let's close this issue when we're done updating the blog post and the documentation
Issue: Discrepancy in `nyu_depth_v2` dataset

Comment:
@awsaf49 thank you for your hard work! 

I am a little unsure why the other links need to be updated, though. They all rely on datasets internally.