Automatically choose dataset split if none provided

[mathemakitten]

Previously, evaluator.compute(..., data='imdb', ....) would fail because it was returning an object of type dataset.DatasetDict. This automatically detects a split if none is given (i.e. user passes in the dataset by name and expects the Evaluator to load it, instead of preloading it themselves).

Closes #226.

[HuggingFaceDocBuilderDev]

The documentation is not available anymore as the PR was closed or merged.

[lvwerra]

That PR is in pretty good shape already!

I wonder if we should setup a dedicated load_dataset_from_string method that is called before prepare_data if a string is provided. Note that the question-answering and token classification have overwritten compute and prepare data so they would also need to be adapted.

Finally, it would be great to add a few tests to make sure this works and doesn't break in the future :)

[mathemakitten]

Hi @ola13, I actually put the original call to the parent class back in and returned the Dataset object instead, since I noticed that both the QA and token classification evaluators use columns from the dataset object in .compute() later. Let me know what you think!

[ola13]

Looks good to me, although I'd wait for @lvwerra the weigh in before merging :)

[lvwerra]

Thanks @mathemakitten for working on this! This looks good in general I have just a concern about the return of prepare_data. It feels a bit odd to return data although we don't always use it. I suggest decoupling the loading from the preparing a bit:

data = self.load_data(data, ...)
metric_inputs, pipeline_inputs = self.prepare_inputs(data, ...)

That way each method is more specialized and we can avoid the x, y, _ = and _, _, z = statements.

I think this will also make overwriting prepare_inputs easier as one can always assume it's a dataset. What do you think?

[lvwerra]

Awesome, generally looks good to me! Mostly left comments regarding the tests. What's left to do:

• refactor tests
• rebase
• make sure CI runs

Then we can merge :)

[lhoestq]

Cool ! a few comments:

[lhoestq]

If you are using load_dataset in your CI, could you set the HF_UPDATE_DOWNLOAD_COUNTS=false environment variable in your CI ? This way it won't count in the download stats on the Hub

[lvwerra]

We have this in the conftest already:

evaluate/tests/conftest.py

Lines 38 to 41 in d5ecbe4

	@pytest.fixture(autouse=True)
	def set_update_download_counts_to_false(monkeypatch):
	# don't take tests into account when counting downloads
	monkeypatch.setattr("evaluate.config.HF_UPDATE_DOWNLOAD_COUNTS", False)

Do you think we need an extra one in the CI?

Edit: This should actually be datasets.config and not evaluate.config, right?

[mathemakitten]

Looking at the other code in config.py just slightly above this, I think evaluate.config is correct?

[lhoestq]

You have to set both ideally ;)

[mathemakitten]

Done :)

[lvwerra]

Thanks for moving the tests! A few things we could add to the tests:

• setup a few small dummy datasets with with different splits (only train, train/test, train/test/valid) and check if the right one is selected by the get_dataset_split method.
• in addition to just checking if there is no error in the test_data_loading we could also check if the data that we get is actually the right one. I would make a differerence between test/train samples and then test if e.g. the first element is the one we expect.
• if you don't mind, could you move the datasets from evaluate-ci to evaluate? there's nothing in that org except the links to the module orgs and the datasets with the images for the docs. so we don't have too many evaluate orgs.

[mathemakitten]

Getting the strangest error in CI here: TestQuestionAnsweringEvaluator.test_model_init is failing in pytest with AssertionError: 33.333333333333336 != 0 but running the test suite in debug mode clearly shows the correct test results as expected, and self.data shows the correct two examples as expected. It almost feels like an environment/configuration error... digging deeper.

edit: resolved by #272

[lvwerra]

Thanks for pushing this over the finish line! Looks great, just two small comments.