fix: fix wrong modification of the 'cache_file_name' -related paramet…
Opened by FrancoisNoyez (open)

…ers values in 'train_test_split' + fix bad interaction between 'keep_in_memory' and 'cache_file_name' -related parameters (#5699)

Comments:
Have you tried to set the cache file names if `keep_in_memory`is True ?

```diff
- if self.cache_files:
+ if self.cache_files and not keep_in_memory:
```

This way it doesn't change the indice cache arguments and leave them as `None`

@lhoestq 
Regarding what you suggest:
The thing is, if cached files already exist and do correspond to the split that we are currently trying to perform, then it would be a shame not to use them, would it not? So I don't think that we should necessarily bypass this step in the method (corresponding to the reading of already existing data), if 'keep_in_memory' = True. For me, 'keep_in_memory' = True is supposed to mean "don't cache the output of this method", but it should say nothing regarding what to do with potentially already existing cached data, should it?
Besides, even if we do what you suggest, and do only that (so, not the modifs that I suggested), then, assuming that 'keep_in_memory' = False and that there exist cached files, if the following check on the existence of cached files with specific name fails, we will still have ended up modifying an input value which will be then used in the remaining of the method, potentially altering the behavior that the user intended the method's call to have. Basically, the issue with what you suggest is that we can't guaranty that we won't continue with the remaining of the method even if this condition is met. Because of that, in my opinion, the best way to not have to worry about potential, unwanted side effects in the rest of the code is to not modify those variables in place, and so, here, to use other variables.
So, I'm sorry, but for those two reasons, I don't think that what you are suggesting addresses the problems which are described in the opened issue.

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

Makes sense ! Therefore removing the ValueError messages sounds good to me, thanks for detailing.

Then I think it's fine to keep using the same variables for the cache file names is enough instead of defining new ones - it doesn't alter the behavior of the function. Otherwise it would feel a bit confusing to have similar variables with slightly modified names just for that

Ok for the removing the ValueError exceptions, thanks.

That said, it seems to me like we should still find a way not to modify the values input by the user, insofar as they can be used elsewhere down the line in the program. Sure, here, by removing the raising of those ValueError exceptions, we have fixed one use cases were allowing this modification actually caused an issue, but maybe there are other use cases where this would also caused an issue? Also, maybe in the future we will add other functionalities which will depend on the values of those input parameters, with then new risks of such an issue occurring?
That's why, in order not to have to worry about that, and in order to make the code a bit more future -proof, I suggest that make sure those input values are not modified.

One way that I did this is to create different but similar looking variable names. If you find this confusing, we can always add a comment.
Another way would be to not store the result of the conditional definition of the values (the '\_cache_file_name = (... if condition else ...)' in my proposition of code), and to use it every time we need. But since we use those new variables at least twice, that creates code redundancy, which is not great either.
Finally, a third way that I can imagine would be to put all this logic into its own method, which would then encapsulate it, and protect the remaining of the 'train_test_split' code from all unintended side effect that this logic can currently cause. This one is probably best. Also, maybe it could be used to remove some code redundancy elsewhere in the definition of the Dataset class? I have not checked if such a code redundancy exists.

We're already replacing the user's input by default values automatically in other methods, it's fine to do it here as well and actually fits the library's style.

Note that the case where it would reload the cache even if `keep_in_memory=True` is not implemented though, but it should be easy to add in `_select_with_indices_mapping`:
- add keep_in_memory in `_new_dataset_with_indices` that uses InMemoryTable.from_file
- inside `_select_with_indices_mapping` return the dataset from `_new_dataset_with_indices` if:
   -  `keep_in_memory=True`
   - and  `indices_cache_file_name` is not None and exists 
   - and `is_caching_enabled()`

Because if we let it this way it would recreate the cache file unfortunately

> We're already replacing the user's input by default values automatically in other methods, it's fine to do it here as well and actually fits the library's style.

I think the fact that it's a style of the library is not really an argument in itself; however, after thinking through it several times, I think I know see why your solution is acceptable: as soon as the user specifies that 'keep_in_memory=True', they should not care anymore about the value of the '\_indices_cache_file_name' variables, since from their point of view those are now irrelevant. So it's "fine" if we allow ourselves to modify the value of those variables, if it helps the internal code being more concise.
Still, I find that it's a bit unintuitive, and a risk as far as future evolution of the method / of the code is concerned; someone tasked with doing that would need to have the knowledge of a lot of, if not all, the other methods of the class, in order to understand the potentially far-reaching impact of some modifications made to this portion of the code. But I guess that's a choice which is the library's owners to make. Also, if we use your proposed solution, as I explained, we can't get the benefit of potentially reusing possibly already existing cached data.
On that note...

> Note that the case where it would reload the cache even if `keep_in_memory=True` is not implemented though

I'm not sure what you mean here:
Within the current code trying to load up the potentially already existing split data, there is no trace of the 'keep_in_memory' variable. So why do you say that 'the case where it would reload the cache even if keep_in_memory=True is not implemented' (I assume that you mean 'currently implemented')? Surely, currently, this bit of code works regardless of the value of the 'keep_in_memory' variable', does it not?