Minimartzz/github-issues huggingface dataset:
Improve inconsistency of `Dataset.map` interface for `load_from_cache_file`
State: closed
Labels: description: New feature or request
name: enhancement

### Feature request


1. Replace the `load_from_cache_file` default value to `True`. 
2. Remove or alter checks from `is_caching_enabled` logic.

### Motivation

I stumbled over an inconsistency in the `Dataset.map` interface. The documentation (and source) states for the parameter `load_from_cache_file`:
```
load_from_cache_file (`bool`, defaults to `True` if caching is enabled):
    If a cache file storing the current computation from `function`
    can be identified, use it instead of recomputing.
```

1. `load_from_cache_file` default value is `None`, while being annotated as `bool`
2. It is inconsistent with other method signatures like `filter`, that have the default value `True`
3. The logic is inconsistent, as the `map` method checks if caching is enabled through `is_caching_enabled`. This logic is not used for other similar methods.

### Your contribution

I am not fully aware of the logic behind caching checks. If this is just a inconsistency that historically grew, I would suggest to remove the `is_caching_enabled` logic as the "default" logic. Maybe someone can give insights, if environment variables have a higher priority than local variables or vice versa. 

If this is clarified, I could adjust the source  according to the "Feature request" section of this issue.

Comments:
Hi, thanks for noticing this! We can't just remove the cache control as this allows us to control where the arrow files generated by the ops are written (cached on disk if enabled or a temporary directory if disabled). The right way to address this inconsistency would be by having `load_from_cache_file=None` by default everywhere.

Hi! Yes, this seems more plausible. I can implement that. One last thing is the type annotation `load_from_cache_file: bool = None`. Which I then would change to `load_from_cache_file: Optional[bool] = None`.

PR #5515 

Yes, `Optional[bool]` is the correct type annotation and thanks for the PR.
Some functions use a param named `type` shouldn't that be avoided since it's a Python reserved name?
State: closed

Hi @mariosasko, @lhoestq, or whoever reads this! :)

After going through `ArrowDataset.set_format` I found out that the `type` param is actually named `type` which is a Python reserved name as you may already know, shouldn't that be renamed to `format_type` before the 3.0.0 is released?

Just wanted to get your input, and if applicable, tackle this issue myself! Thanks 🤗

Comments:
Hi! Let's not do this - renaming it would be a breaking change, and going through the deprecation cycle is only worth it if it improves user experience.

Hi @mariosasko, ok it makes sense. Anyway, don't you think it's worth it at some point to start a deprecation cycle e.g. `fs` in `load_from_disk`? It doesn't affect user experience but it's for sure a bad practice IMO, but's up to you 😄 Feel free to close this issue otherwise!

I don't think deprecating a param name in this particular instance is worth the hassle, so I'm closing the issue 🙂.

Sure, makes sense @mariosasko thanks!
Milvus integration for search
State: open

Signed-off-by: Filip Haltmayer <filip.haltmayer@zilliz.com>

Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/datasets/pr_5510). All of your documentation changes will be reflected on that endpoint.

To the maintainer, sorry about the repeated run requests for formatting. Missed the `make style` outlined in contributing guidelines. 

Anything I can do to get the workflow to run? @lhoestq 

cc @mariosasko 

> Anything I can do to get the workflow to run?

You can merge `main` into your branch to fix code formatting (we switched from isort+flake8 to ruff this week), and then run `make style`

I believe that should be good. @mariosasko
Add a static `__all__` to `__init__.py` for typecheckers
State: open

This adds a static `__all__` field to `__init__.py`, allowing typecheckers to know which symbols are accessible from `datasets` at runtime. In particular [Pyright](https://github.com/microsoft/pylance-release/issues/2328#issuecomment-1029381258) seems to rely on this. At this point I have added all (modulo oversight) the symbols mentioned in the Reference part of [the docs](https://huggingface.co/docs/datasets), but that could be adjusted. As a side effect, only these symbols will be imported by `from datasets import *`, which may or may not be a good thing (and if it isn't, that's easy to fix).

Another option would be to add a pyi stub, but I think `__all__` should be the most pythonic solution.

This should fix #3841.

Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/datasets/pr_5509). All of your documentation changes will be reflected on that endpoint.

Hi! I've commented on the original issue to provide some context. Feel free to share your opinion there.
Saving a dataset after setting format to torch doesn't work, but only if filtering
State: closed

### Describe the bug

Saving a dataset after setting format to torch doesn't work, but only if filtering

### Steps to reproduce the bug

```
a = Dataset.from_dict({"b": [1, 2]})
a.set_format('torch')
a.save_to_disk("test_save") # saves successfully
a.filter(None).save_to_disk("test_save_filter") # does not
>> [...] TypeError: Provided `function` which is applied to all elements of table returns a `dict` of types [<class 'torch.Tensor'>]. When using `batched=True`, make sure provided `function` returns a `dict` of types like `(<class 'list'>, <class 'numpy.ndarray'>)`.

# note: skipping the format change to torch lets this work.



### Expected behavior

Saving to work

### Environment info

- `datasets` version: 2.4.0
- Platform: Linux-6.1.9-arch1-1-x86_64-with-glibc2.36
- Python version: 3.10.9
- PyArrow version: 9.0.0
- Pandas version: 1.4.4

Comments:
Hey, I'm a research engineer working on language modelling wanting to contribute to open source. I was wondering if I could give it a shot?

Hi! This issue was fixed in https://github.com/huggingface/datasets/pull/4972, so please install `datasets>=2.5.0` to avoid it.
Optimise behaviour in respect to indices mapping
State: open
Labels: description: New feature or request
name: enhancement

_Originally [posted](https://huggingface.slack.com/archives/C02V51Q3800/p1675443873878489?thread_ts=1675418893.373479&cid=C02V51Q3800) on Slack_

Considering all this, perhaps for Datasets 3.0, we can do the following:
* [ ] have `continuous=True` by default in `.shard`  (requested in the survey and makes more sense for us since it doesn't create an indices mapping)
* [x] allow calling `save_to_disk` on "unflattened" datasets
* [ ] remove "hidden" expensive calls in `save_to_disk`, `unique`, `concatenate_datasets`, etc. For instance, instead of silently calling `flatten_indices`  where it's needed, it's probably better to be explicit (considering how expensive these ops can be) and raise an error instead

Comments:
PyTorch BatchSampler still loads from Dataset one-by-one
State: closed

### Describe the bug

In [the docs here](https://huggingface.co/docs/datasets/use_with_pytorch#use-a-batchsampler), it mentions the issue of the Dataset being read one-by-one, then states that using a BatchSampler resolves the issue.

I'm not sure if this is a mistake in the docs or the code, but it seems that the only way for a Dataset to be passed a list of indexes  by PyTorch (instead of one index at a time) is to define a `__getitems__` method (note the plural) on the Dataset object, and since the HF Dataset doesn't have this, PyTorch executes [this line of code](https://github.com/pytorch/pytorch/blob/master/torch/utils/data/_utils/fetch.py#L58), reverting to fetching one-by-one.

### Steps to reproduce the bug

You can put a breakpoint in `Dataset.__getitem__()` or just print the args from there and see that it's called multiple times for a single `next(iter(dataloader))`, even when using the code from the docs:
```py
from torch.utils.data.sampler import BatchSampler, RandomSampler
batch_sampler = BatchSampler(RandomSampler(ds), batch_size=32, drop_last=False)
dataloader = DataLoader(ds, batch_sampler=batch_sampler)
```



### Expected behavior

The expected behaviour would be for it to fetch batches from the dataset, rather than one-by-one.

To demonstrate that there is room for improvement: once I have a HF dataset `ds`, if I just add this line:
```py
ds.__getitems__ = ds.__getitem__
```
...then the time taken to loop over the dataset improves considerably (for wikitext-103, from one minute to 13 seconds with batch size 32). Probably not a big deal in the grand scheme of things, but seems like an easy win.

### Environment info

- `datasets` version: 2.9.0
- Platform: Linux-5.10.102.1-microsoft-standard-WSL2-x86_64-with-glibc2.31
- Python version: 3.10.8
- PyArrow version: 10.0.1
- Pandas version: 1.5.3

Comments:
This change seems to come from a few months ago in the PyTorch side. That's good news and it means we may not need to pass a batch_sampler as soon as we add `Dataset.__getitems__` to get the optimal speed :)

Thanks for reporting ! Would you like to open a PR to add `__getitems__` and remove this outdated documentation ?

Yeah I figured this was the sort of thing that probably once worked. I can confirm that you no longer need the batch sampler, just `batch_size=n` in the `DataLoader`.

I'll pass on the PR, I'm flat out right now, sorry.
`load_dataset` has ~4 seconds of overhead for cached data
State: open
Labels: description: New feature or request
name: enhancement

### Feature request

When loading a dataset that has been cached locally, the `load_dataset` function takes a lot longer than it should take to fetch the dataset from disk (or memory).
This is particularly noticeable for smaller datasets. For example, wikitext-2, comparing `load_data` (once cached) and `load_from_disk`, the `load_dataset` method takes 40 times longer.

⏱    4.84s ⮜ load_dataset
⏱    119ms ⮜ load_from_disk

### Motivation

I assume this is doing something like checking for a newer version.

If so, that's an age old problem: do you make the user wait _every single time they load from cache_ or do you do something like load from cache always, _then_ check for a newer version and alert if they have stale data. The decision usually revolves around what percentage of the time the data will have been updated, and how dangerous old data is.

For most datasets it's extremely unlikely that there will be a newer version on any given run, so 99% of the time this is just wasted time.

Maybe you don't want to make that decision for all users, but at least having the _option_ to not wait for checks would be an improvement.

### Your contribution

.

Comments:
Hi ! To skip the verification step that checks if newer data exist, you can enable offline mode with `HF_DATASETS_OFFLINE=1`.

Although I agree this step should be much faster for datasets hosted on the HF Hub - we could just compare the commit hash from the local data and the remote git repository. We're not been leveraging the git commit hashes, since the library was built before we even had git repositories for each dataset on HF.

Thanks @lhoestq, for memory when I recorded those times I had `HF_DATASETS_OFFLINE` set.
TypeError: 'bool' object is not iterable when filtering a datasets.arrow_dataset.Dataset
State: closed

### Describe the bug

Hi, 

Thanks for the amazing work on the library!

**Describe the bug**

I think I might have noticed a small bug in the filter method. 
Having loaded a dataset using `load_dataset`,  when I try to filter out empty entries with `batched=True`, I get a TypeError.

### Steps to reproduce the bug

```
train_dataset = train_dataset.filter(
            function=lambda example: example["image"] is not None, 
            batched=True,
            batch_size=10)
```
Error message:

```
File .../lib/python3.9/site-packages/datasets/fingerprint.py:480, in fingerprint_transform.<locals>._fingerprint.<locals>.wrapper(*args, **kwargs)
    476             validate_fingerprint(kwargs[fingerprint_name])
    478 # Call actual function
--> 480 out = func(self, *args, **kwargs)
...
-> 5666 indices_array = [i for i, to_keep in zip(indices, mask) if to_keep]
   5667 if indices_mapping is not None:
   5668     indices_array = pa.array(indices_array, type=pa.uint64())

TypeError: 'bool' object is not iterable
```

**Removing batched=True allows to bypass the issue.** 

### Expected behavior

According to the doc, "[batch_size corresponds to the] number of examples per batch provided to function if batched = True", so we shouldn't need to remove the batchd=True arg?

source: https://huggingface.co/docs/datasets/v2.9.0/en/package_reference/main_classes#datasets.Dataset.filter

### Environment info

- `datasets` version: 2.9.0
- Platform: Linux-5.4.0-122-generic-x86_64-with-glibc2.31
- Python version: 3.9.10
- PyArrow version: 10.0.1
- Pandas version: 1.5.3

Comments:
Hi! Instead of a single boolean, your filter function should return an iterable (of booleans) in the batched mode like so:
```python
train_dataset = train_dataset.filter(
            function=lambda batch: [image is not None for image in batch["image"]], 
            batched=True,
            batch_size=10)
```

PS: You can make this operation much faster by operating directly on the arrow data to skip the decoding part:
```python
train_dataset = train_dataset.with_format("arrow")
train_dataset = train_dataset.filter(
            function=lambda table: table["image"].is_valid().to_pylist(), 
            batched=True,
            batch_size=100)
train_dataset = train_dataset.with_format(None)
```

Thank a lot!

I hit the same issue and the error message isn't really clear on what's going wrong. It might be helpful to update the docs with a batched example.
Add a `reduce` method
State: closed
Labels: description: New feature or request
name: enhancement

### Feature request

Right now the `Dataset` class implements `map()` and `filter()`, but leaves out the third functional idiom popular among Python users: `reduce`. 

### Motivation

A `reduce` method is often useful when calculating dataset statistics, for example, the occurrence of a particular n-gram or the average line length of a code dataset.  

### Your contribution

I haven't contributed to `datasets` before, but I don't expect this will be too difficult, since the implementation will closely follow that of `map` and `filter`. I could have a crack over the weekend. 

Comments:
Hi! Sure, feel free to open a PR, so we can see the API you have in mind.

I would like to give it a go! #self-assign

Closing as `Dataset.map` can be used instead (see https://github.com/huggingface/datasets/pull/5533#issuecomment-1440571658 and https://github.com/huggingface/datasets/pull/5533#issuecomment-1446403263)

Hello, is it possible for this issue/PR to be revisited? The problem with the alternatives presented (besides multiple map stages) is that they don't use the cache. A reduce operation is just as expensive as a map operation because it also goes over the entire dataset. It's equally worth caching.

Personally, I have a situation where I would need this and map is far from ideal. I'm working on updating a project of mine to use Huggingface Datasets, and I need to port the loop at https://github.com/colonelwatch/abstracts-search/blob/b90f31ee4cc6e394f829d3a6d9d0311ca390ada9/train.py#L112-L138. Please forgive the code style, here's what it does in English. I have a dataset of about 95 million embeddings, out of which 16384 is taken as a "query" set. For each embedding in the query set, I need to find the ten closest neighbors. These nearest neighbors are used to tune the parameters of a faiss index. The solution is to set up an "accumulator" comprising of the ten closest so far and their distances, then do a single scan over the 95 million (memmapped), then save the results of the "accumulator" for when I want to prototype another index.

The closest approximation to this is multiple map stages, but with such a large "accumulator" having the RAM to do a big batch size becomes critical. At a batch size of 1000, the intermediate accumulators would in theory be about 120 GB! That can be more if I want higher precision than float32. It would already be about the same size as the original embeddings. Using larger batch sizes puts strain on the RAM because I'd be dealing with batch_size x 16384 distances. The best I'd gotten with my RAM, single-threaded, was 65536, and for speed I had to use that thread to feed a GPU. It'd be better if I could use multiple threads to get high throughput instead, or even do all the work in CPU, but to fit the threads I'd need the batch size to be smaller.

All of this intermediate memory could be eliminated if there was a reduce operation.
Update audio installation doc page
State: closed
Labels: description: Improvements or additions to documentation
name: documentation

Our [installation documentation page](https://huggingface.co/docs/datasets/installation#audio) says that one can use Datasets for mp3 only with `torchaudio<0.12`. `torchaudio>0.12` is actually supported too but requires a specific version of ffmpeg which is not easily installed on all linux versions but there is a custom ubuntu repo for it, we have insctructions in the code: https://github.com/huggingface/datasets/blob/main/src/datasets/features/audio.py#L327 
So we should update the doc page. But first investigate [this issue](5488). 

Comments:
Totally agree, the docs should be in sync with our code.

Indeed to avoid confusing users, I think we should have updated the docs at the same time as this PR:
- #5167

@albertvillanova yeah sure I should have, but I forgot back then, sorry for that 😶

No, @polinaeterna, nothing to be sorry about.

My comment was for all of us datasets team, as a reminder: when making a PR, but also when reviewing some other's PR, we should not forget to update the corresponding docstring and doc pages. It is something we can improve if we help each other in reminding about it... :hugs: 

@polinaeterna I think we can close this issue now as we no longer use `torchaudio` for decoding.
Push_to_hub in a pull request
State: closed
Labels: description: New feature or request
name: enhancement

description: Good for newcomers
name: good first issue

Right now `ds.push_to_hub()` can push a dataset on `main` or on a new branch with `branch=`, but there is no way to open a pull request. Even passing `branch=refs/pr/x` doesn't seem to work: it tries to create a branch with that name

cc @nateraw 

It should be possible to tweak the use of `huggingface_hub` in `push_to_hub` to make it open a PR or push to an existing PR

Comments:
Assigned to myself and will get to it in the next week, but if someone finds this issue annoying and wants to submit a PR before I do, just ping me here and I'll reassign :). 

I would like to be assigned to this issue, @nateraw . #self-assign
Adding `sep` to TextConfig 
State: open

I have a local a `.txt` file that follows the `CONLL2003` format which I need to load using `load_script`. However, by using `sample_by='line'`, one can only split the dataset into lines without splitting each line into columns. Would it be reasonable to add a `sep` argument in combination with `sample_by='paragraph'` to parse a paragraph into an array for each column ? If so, I am happy to contribute!

## Environment
* `python 3.8.10`
* `datasets 2.9.0`

## Snippet of `train.txt`

```txt
Distribution	NN	O	O
and	NN	O	O
dynamics	NN	O	O
of	NN	O	O
electron	NN	O	B-RP
complexes	NN	O	I-RP
in	NN	O	O
cyanobacterial	NN	O	B-R
membranes	NN	O	I-R

The	NN	O	O
occurrence	NN	O	O
of	NN	O	O
prostaglandin	NN	O	B-R
F2α	NN	O	I-R
in	NN	O	O
Pharbitis	NN	O	B-R
seedlings	NN	O	I-R
grown	NN	O	O
under	NN	O	O
short	NN	O	B-P
days	NN	O	I-P
or	NN	O	I-P
days	NN	O	I-P

```

## Current Behaviour 
```python
# defining 4 features ['tokens', 'pos_tags', 'chunk_tags', 'ner_tags'] here would fail with `ValueError: Length of names (4) does not match length of arrays (1)`
dataset = datasets.load_dataset(path='text', features=features, data_files={'train': 'train.txt'}, sample_by='line')
dataset['train']['tokens'][0]
>>> 'Distribution\tNN\tO\tO'
```

## Expected Behaviour / Suggestion
```python
# suppose we defined 4 features ['tokens', 'pos_tags', 'chunk_tags', 'ner_tags']
dataset = datasets.load_dataset(path='text', features=features, data_files={'train': 'train.txt'}, sample_by='paragraph', sep='\t')
dataset['train']['tokens'][0]
>>> ['Distribution', 'and', 'dynamics', ... ]

dataset['train']['ner_tags'][0]
>>> ['O', 'O', 'O', ... ]
```

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

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 ?
Reload features from Parquet metadata
State: closed
Labels: description: New feature or request
name: enhancement

description: Issues a bit more difficult than "Good First" issues
name: good second issue

The idea would be to allow this :
```python 
ds.to_parquet("my_dataset/ds.parquet")
reloaded = load_dataset("my_dataset")
assert ds.features == reloaded.features 
```
And it should also work with Image and Audio types (right now they're reloaded as a dict type)

This can be implemented by storing and reading the feature types in the parquet metadata, as we do for arrow files.

Comments:
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?


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

#self-assign