Skip to content

Comments

[MNLI example] Prevent overwriting matched with mismatched metrics#16475

Merged
sgugger merged 2 commits intohuggingface:mainfrom
eldarkurtic:fix-mnli-example
Mar 29, 2022
Merged

[MNLI example] Prevent overwriting matched with mismatched metrics#16475
sgugger merged 2 commits intohuggingface:mainfrom
eldarkurtic:fix-mnli-example

Conversation

@eldarkurtic
Copy link
Contributor

What does this PR do?

Fixes #16474.

The fix appends _mm to the metrics for mnli-mm evaluation dataset to prevent overwriting eval metrics from the mnli dataset, and writes them together in eval_results.json and all_results.json. The MNLI-metrics look like this:

{
    "eval_accuracy": 0.36067244014263883,
    "eval_accuracy_mm": 0.35506509357200977,
    "eval_loss": 1.158889889717102,
    "eval_loss_mm": 1.1670204401016235,
    "eval_runtime": 16.4691,
    "eval_runtime_mm": 15.9496,
    "eval_samples": 9815,
    "eval_samples_mm": 9832,
    "eval_samples_per_second": 595.964,
    "eval_samples_per_second_mm": 616.44,
    "eval_steps_per_second": 18.641,
    "eval_steps_per_second_mm": 19.311
}

@sgugger, @patil-suraj

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 29, 2022

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

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your PR! I just have two comments and it should be good to merged.

Also make sur to run make style on your branch when you're done so the quality check passes.

Comment on lines +522 to +525
if task == "mnli-mm":
metrics = {k + "_mm": v for k, v in metrics.items()}
if "mnli" in task:
combined.update(metrics)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd regroup this slightly differently:

Suggested change
if task == "mnli-mm":
metrics = {k + "_mm": v for k, v in metrics.items()}
if "mnli" in task:
combined.update(metrics)
if task == "mnli-mm":
combined.update({k + "_mm": v for k, v in metrics.items()})
elif task == "mnli":
combined.update(metrics)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this regrouping, the trainer.log_metrics("eval", metrics) won't have the "_mm" , so the stdout will be the same as before (that's the main reason why keys in metrics are updated). Given this, should we still regroup and affect only the json log files, but not the stdout?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah understood. LGTM as is then :-)

@sgugger
Copy link
Collaborator

sgugger commented Mar 29, 2022

The CI failure is unrelated to this PR (actually investigating it right now), so we can merge safely :-)

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.

MNLI metrics overwritten

3 participants