Update run_glue for do_predict with local test data (#9442) by forest1988 · Pull Request #9486 · huggingface/transformers

forest1988 · 2021-01-08T17:44:16Z

What does this PR do?

Currently, run_glue.py cannot use the test set (do_predict) unless we give it a GLUE task name.
This PR will allow us to use the local test dataset.

As commented in #9442, I tried to achieve the functionality with only simple changes.

It still works with only the local train and valid files (in other words, this PR does not break the current operation.).
If we add --do_predict with out adding specific params, we will get an error statement saying that we need either the GLUE task name or the path of the local test file.

Fixes #9442

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guideline,
Pull Request section?
Was this discussed/approved via a Github issue or the forum? Please add a link
to it if that's the case.
Did you make sure to update the documentation with your changes? Here are the
documentation guidelines, and
here are tips on formatting docstrings.
Did you write any new necessary tests?

Who can review?

@sgugger

Thank you for your kind comments on the issue.
I have tried to keep it simple and hope there is no problem as an example script.

forest1988 · 2021-01-08T18:06:27Z

Error messages of the CircleCI are:

-- Docs: https://docs.pytest.org/en/stable/warnings.html
=========================== short test summary info ============================
FAILED tests/test_pipelines_conversational.py::SimpleConversationPipelineTests::test_history_cache
FAILED tests/test_pipelines_conversational.py::SimpleConversationPipelineTests::test_integration_torch_conversation
==== 2 failed, 4207 passed, 1744 skipped, 734 warnings in 190.84s (0:03:10) ====

FAILED tests/test_pipelines_conversational.py::SimpleConversationPipelineTests::test_history_cache
==== 1 failed, 4178 passed, 1774 skipped, 735 warnings in 260.31s (0:04:20) ====

I'm sorry but I'd like to ask you if run_glue.py is related to the conversation pipeline.

sgugger

The failures seems spurious indeed. I've left some comment to move the test dataset creation a bit, but it overall looks good to me.

sgugger · 2021-01-08T18:12:52Z

+        if data_args.task_name is None and data_args.test_file is not None:
+            extension = data_args.test_file.split(".")[-1]
+            assert extension in ["csv", "json"], "`test_file` should be a csv or a json file."
+            if data_args.test_file.endswith(".csv"):
+                # Loading a dataset from a local csv file
+                test_dataset = load_dataset("csv", data_files={"test": data_args.test_file})
+            else:
+                # Loading a dataset from a local json file
+                test_dataset = load_dataset("json", data_files={"test": data_args.test_file})


Can we put those lines earlier, with the validation dataset? This way the map will be done with the other dataset. I think we can do something nice by creating data_files={"train": data_args.train_file, "validation": data_args.validation_file} and then adding the keys test if the test_file is passed.

Thank you for your comment! I've reflected the review.
The nested if statements in the code have increased, but I think the readability may have improved in terms of "whether to use GLUE task or to use local files". What do you think?

I also added the logger output to make sure that the local files a user wants to use are loaded correctly. If this is superfluous, please let me know and I will remove it.

sgugger

Looking good! Left two more small comments and it should be good to merge!

sgugger · 2021-01-11T14:10:58Z

+        if training_args.do_predict:
+            if data_args.test_file is not None:
+                extension = data_args.test_file.split(".")[-1]
+                assert extension in ["csv", "json"], "`test_file` should be a csv or a json file."


The extension will need to be the same one as for the training and validation file, so we should adapt this assert to test that.

Reflecting the comments, assert now checks that the test file has the same extension as the train file.
Also, I thought there was no check if the validation file has the same extension as the train file, so I modified that. Is this change OK?

sgugger · 2021-01-11T14:11:17Z

+            datasets = load_dataset(
+                "json", data_files=data_files
+            )


Suggested change

datasets = load_dataset(

"json", data_files=data_files

)

datasets = load_dataset("json", data_files=data_files)

Can fit in one line now :-)

Thank you!　
It may be that the old code before applying the auto-format was left in place.
I have applied the auto-format in b2936c3, could you please check if it is fit in one line?

sgugger · 2021-01-11T14:12:31Z

+        for key in data_files.keys():
+            logger.info(f"load a local file for {key}: {data_files[key]}")


This could info to log, thanks for adding!

LysandreJik

Great, thanks for adding!

forest1988 · 2021-01-13T15:45:16Z

@sgugger @LysandreJik
Thank you for reviewing and merging!

forest1988 added 2 commits January 9, 2021 02:29

Update run_glue for do_predict with local test data (huggingface#9442)

dba9600

Update run_glue (huggingface#9442): fix comments ('files' to 'a file')

a77675e

sgugger approved these changes Jan 8, 2021

View reviewed changes

forest1988 added 2 commits January 9, 2021 07:24

Update run_glue (huggingface#9442): reflect the code review

b1e10ac

Update run_glue (huggingface#9442): auto format

b2936c3

sgugger approved these changes Jan 11, 2021

View reviewed changes

sgugger reviewed Jan 11, 2021

View reviewed changes

sgugger requested a review from LysandreJik January 11, 2021 14:12

Update run_glue (huggingface#9442): reflect the code review

37531ab

LysandreJik approved these changes Jan 13, 2021

View reviewed changes

sgugger merged commit eabad8f into huggingface:master Jan 13, 2021

		for key in data_files.keys():
		logger.info(f"load a local file for {key}: {data_files[key]}")

Conversation

forest1988 commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Before submitting

Who can review?

Uh oh!

forest1988 commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgugger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgugger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LysandreJik left a comment

Choose a reason for hiding this comment

Uh oh!

forest1988 commented Jan 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

forest1988 commented Jan 8, 2021 •

edited

Loading

forest1988 commented Jan 8, 2021 •

edited

Loading