Skip to content

Add step to download dataset before running the preprocess_data_dist test - #81

Merged
stas00 merged 3 commits into
bigscience-workshop:mainfrom
adammoody:disttest
Aug 27, 2021
Merged

Add step to download dataset before running the preprocess_data_dist test#81
stas00 merged 3 commits into
bigscience-workshop:mainfrom
adammoody:disttest

Conversation

@adammoody

Copy link
Copy Markdown
Contributor

No description provided.

@adammoody

Copy link
Copy Markdown
Contributor Author

@stas00 , @thomasw21 , this adds a step to download the dataset before running the preprocess_data_dist test.

We decided to have that script bail out by default if the dataset is not already downloaded. It is assumed that one is using it to preprocess a large dataset and likely using a lot of compute nodes. Since a large dataset will take a long time to download, this would be inefficient use of those nodes, as they would all be sitting idle waiting for the dataset to be downloaded and extracted. One can override that default behavior by setting HF_DATASETS_OFFLINE=0.

This PR adds an extra step to download the dataset using a simple program that just calls load_dataset and exits.

@adammoody

Copy link
Copy Markdown
Contributor Author

Also, I think that if one does have the dataset pre-downloaded, then openwebtext-10k works in place of stas/openwebtext-10k. In my dataset directory, it seems to be unpacked into ${HF_DATASETS_CACHE}/openwebtext-10k instead of ${HF_DATASETS_CACHE}/stas/openwebtext-10k.

Do others see the same?

I don't know whether that's an intentional feature or whether it's a bug. It seems like it would collide if there happened to be a dataset named something like adam/openwebtext-10k.

@stas00

stas00 commented Aug 26, 2021

Copy link
Copy Markdown
Contributor

This looks good, but any reason to do it in a separate process? Isn't it going to be just fine to do it directly inside pytest?

and please rebase it, as I pushed an earlier fix to this. Thank you!

@stas00

stas00 commented Aug 26, 2021

Copy link
Copy Markdown
Contributor

Also, I think that if one does have the dataset pre-downloaded, then openwebtext-10k works in place of stas/openwebtext-10k. In my dataset directory, it seems to be unpacked into ${HF_DATASETS_CACHE}/openwebtext-10k instead of ${HF_DATASETS_CACHE}/stas/openwebtext-10k.

Do others see the same?

I don't know whether that's an intentional feature or whether it's a bug. It seems like it would collide if there happened to be a dataset named something like adam/openwebtext-10k.

This does sound like it would be a potential bug. But it uses a long hash dir before or after the dataset name, so most likely that's how it creates a unique dataset. So it looks up stas/openwebtext-10k hashes that and that's how it knows to pick the location of
stas/openwebtext-10k over adam/openwebtext-10k I think.

e.g. see on my machine:

cache/huggingface/modules/datasets_modules/datasets/openwebtext-10k/ae51b9a89a05b825d6d18b9267c2ba1cf7faeb242bbe3144cf6cb62d92f05d81
cache/huggingface/datasets/downloads/extracted/f1001b035aca8fda879b20cbb26a4de474f2bf71cddb91ab202da8fc2d6f1b14/openwebtext-10k

Are you also saying it doesn't work if you pass stas/openwebtext-10k when the dataset has been downloaded already?

@adammoody

Copy link
Copy Markdown
Contributor Author

Ok, rebased and updated to load the dataset directly. Let me know if that helps.

@adammoody

Copy link
Copy Markdown
Contributor Author

Are you also saying it doesn't work if you pass stas/openwebtext-10k when the dataset has been downloaded already?

It does work with the full name, and I've changed the test to use the full name. I was just surprised to find that it had been working without using the full name when you pointed it out.

@stas00

stas00 commented Aug 26, 2021

Copy link
Copy Markdown
Contributor

Yes, I have been bitten by this one too. Let's report this to datasets

@adammoody

Copy link
Copy Markdown
Contributor Author

I left this line in place:

@unittest.skip("Skip test until data is fixed.")

@stas00

stas00 commented Aug 26, 2021

Copy link
Copy Markdown
Contributor

Filed a feature request huggingface/datasets#2842

Comment thread tests/test_preprocessing.py Outdated
@stas00
stas00 merged commit fa9f54c into bigscience-workshop:main Aug 27, 2021
@stas00

stas00 commented Aug 27, 2021

Copy link
Copy Markdown
Contributor

Hoping we will have a live CI really soon now.

@adammoody
adammoody deleted the disttest branch August 27, 2021 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants