pre_tokenization_hook is called on the Dataset before tokenization and should return a modified Dataset object. The intended use is for carrying out tasks that require modifying the dataset as a whole as opposed to modifying individual datapoints, for example discarding certain datapoints based on a particular condition. Tokenization and other "elementwise" operations on the data are performed by the process function which is mapped over the dataset.

See comment below, this is passed on to the load_dataset function.

Do we want to commit all of these arrow files etc. to the repo?

It's a dummy dataset. I agree it isn't perfect but I'm not sure what choice we have. could we zip it?

Vision has something of similar taste
https://github.com/pytorch/vision/tree/main/test/assets
I can put it all in a .pt

this is only used internally? should we put a "_" in front?

Is it? IDK if we want to expose it or not. To me it's a well self-contained function properly documented that could be used somewhere else, but I'm happy to make it private if there's no use.

I think in the common case user will want to (possibly) create and load. Hence my proposal to keep this internal and expose only the maybe-renamed other function

what about calling this just "load_dataset" and maybe add an argument such as "exists" in mkdir?

not sure i see what you mean but the reason I named it that way is that it can either create a dataset for the first time or load it if the memmap tensordict copy is already there. load_dataset already exists and does just that, nothing more.

My suggestion/discussion is whether we could rename this to load_dataset without changing it's current behaviour and add an argument to specify the behaviour we want in case it does not exists.

Im fine with renaming it, though I think that users should be informed that this function caches the results and the name says that quite explicitly.
I'm not sure I see what the extra arg should do. If it does not exist we must load it, and if it does we should use the cache. What extra control should we give?

just a flag like exists_ok of mkdir that raises if it does not exists or something similar to from_disk but control whether we want to store cache or not.
I don't have strong opinion on this I am just reasoning if it's really necessary such a long name (even if definitely more explicit) or if it will be enough to say that we cache in the doc.

Instead of doing this in every test that loads the data, perhaps we should write a conftest.py file that extracts on setup, then cleans up on completion?

Good point yeah
But then how do we tell conftest to do it only when the test_rlhf.py script is executed?

But then how do we tell conftest to do it only when the test_rlhf.py script is executed?

How about instead of conftest.py we add a fixture that has session scope and yields a directory which is the location of the extracted data? I think the fixture should only be executed if it is used, and it will only be executed once.

I think fixtures are executed in each test, otherwise the same temporary directories would be reused multiple times

Yes, that's the default, but you can change that behaviour

@pytest.fixture(scope="session")
def minidata_dir():
    # unzip
    # yield path

Oh now i remember why i did not do that
we have 2 mini datasets but you can't pass a fixture to parametrize, so I couldn't write 2 different fixtures for the tests
The "ugly" solution would be

@pytest.mark.parametrize("dataset", ["comp", "prompt"])
def test_stuff(dataset, comp_fixture, prompt_fixture):
    if dataset == "comp":
        dataset = comp_fixture
    else:
        dataset = prompt_fixture

Should we add logic here to zip the generated data so that the committed .zip files are reproducible?

I did not use python for that but we could do it

this

got it
Let's parametrize it then

tokenizers already have a method tokenize. maybe it's better to be more explicit about what this does