Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update doc for metric_for_best_model when save_strategy="best". #35389

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Update doc for metric_for_best_model when save_strategy="best". #35389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
048250e
Updated docstring for _determine_best_metric.
seanswyi Dec 22, 2024
a903136
Updated docstring for metric_for_best_model.
seanswyi Dec 22, 2024
84e91d1
Added test case for save strategy.
seanswyi Dec 22, 2024
8accf30
Updated incorrect test case.
seanswyi Dec 22, 2024
20b276a
Changed eval_strategy to match save_strategy.
seanswyi Dec 22, 2024
dd79c84
Merge branch 'main' into fix/update-metric-for-best-model-default
seanswyi Dec 28, 2024
ce46012
Separated test cases for metric.
seanswyi Dec 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/transformers/trainer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3156,7 +3156,6 @@ def _load_rng_state(self, checkpoint):
def _determine_best_metric(self, metrics, trial):
"""
Determine if the model should be saved based on the evaluation metrics.
If args.metric_for_best_model is not set, the loss is used.

Returns:
bool: True if a new best metric was found, else False
Expand Down
2 changes: 1 addition & 1 deletion src/transformers/training_args.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ class TrainingArguments:
metric_for_best_model (`str`, *optional*):
Use in conjunction with `load_best_model_at_end` to specify the metric to use to compare two different
models. Must be the name of a metric returned by the evaluation with or without the prefix `"eval_"`. Will
default to `"loss"` if unspecified and `load_best_model_at_end=True` (to use the evaluation loss).
default to `"loss"` if unspecified, `load_best_model_at_end=True`, and `save_strategy!="best"`.
Copy link
Contributor

Choose a reason for hiding this comment

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

my 2cs (Disclaimer! I'm not very familiar with the whole scope of the initial change, or reason behind it!): it's a bit hard to read and understand what is going on here and why. E.g. why can't it default to loss when save_strategy == best? What is the major difference with the load_best_model_at_end (and save_strategy!="best")?

Again, apologies if I'm missing some obvious context here. Please feel free to disregard my comment / question then.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't find the place where we set metric_for_best_model = "loss" when save_strategy!=best. Can you explain a bit why you changed the description ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shcheklein That was a design decision made here (#31817 (comment)). It was deemed easier to debug if we don't add a hard-coded value and rather raise an error.

@SunMarc Hmm I'm starting to think that maybe the problem is that we're not able to set load_best_model_at_end = True when save_strategy = "best" since load_best_model_at_end requires eval_strategy == save_strategy but eval_strategy doesn't have a "best" option.

This means that if we want to use save_strategy = "best" then we have to have load_best_model_at_end = False, which in turn means that when save_strategy != "best" and load_best_model_at_end = True then the __post_init__ method of TrainingArguments is setting metric_for_best_model to "loss". https://github.com/huggingface/transformers/blob/main/src/transformers/training_args.py#L1676:L1679

The modified docstring aims to add a bit more detail as to when the metric_for_best_model is set to a default of "loss".

Copy link
Member

Choose a reason for hiding this comment

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

should we also add best for eval_strategy then ?

Copy link
Contributor Author

@seanswyi seanswyi Dec 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like that might sound a bit awkward since it means we'd "perform evaluation at every best checkpoint."

Maybe we could check if save_strategy == "best" and then bypass the eval_strategy == save_strategy condition? That would mean that here we would change the code to:

if self.load_best_model_at_end and self.save_strategy != "best":
    if self.eval_strategy != self.save_strategy:
        raise ValueError(
            "--load_best_model_at_end requires the save and eval strategy to match, but found\n- Evaluation "
            f"strategy: {self.eval_strategy}\n- Save strategy: {self.save_strategy}"
        )

I'm not 100% sure about the history of why eval_strategy == save_strategy but I'm assuming that it's a safe guard to prevent situations where we want to load the best model at the end of training but we never saved it because the two didn't match. If save_strategy == "best" I don't think we'd have that problem since saving is guaranteed to following evaluation.

I think this also means that we may have to either remove the default loss error we're raising or change it to a warning (#31817 (comment)).

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me for the new condition, just add a comment to explain why we don't need to perform the check when self.save_strategy == "best". Also, which default loss error you are talking about ? I'm not sure why we need to remove it.


If you set this value, `greater_is_better` will default to `True`. Don't forget to set it to `False` if
your metric is better when lower.
Expand Down
19 changes: 17 additions & 2 deletions tests/trainer/test_trainer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4220,7 +4220,9 @@ def test_save_best_checkpoint(self):
total=total,
)

# Case 3: Metric name not provided; throw error.
def test_metric_for_best_model_behavior(self):
# Case 1: Metric name not provided when `save_strategy == "best"`.
# Should raise ValueError.
with tempfile.TemporaryDirectory() as tmpdir:
with self.assertRaises(ValueError) as context:
trainer = get_regression_trainer(
Expand All @@ -4232,9 +4234,22 @@ def test_save_best_checkpoint(self):
save_strategy="best",
compute_metrics=AlmostAccuracy(),
)

self.assertIn("`args.metric_for_best_model` must be provided", str(context.exception))

# Case 2: Metric name not provided when `load_best_model_at_end == True`.
# `metric_for_best_model` should be set to `"loss"` by default.
with tempfile.TemporaryDirectory() as tmpdir:
Copy link
Contributor

Choose a reason for hiding this comment

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

not critical / minor: tbh, it seems a bit out of place for the test_save_best_checkpoint (as well as the previous case). I would probably move it into a separate test. Or should it otherwise call at least train and test actual checkpoint saved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I agree that it logically does seem a bit out of place. I think cases 3 and 4 could be grouped into their own methods since the point isn't so much to test the save_strategy = "best" itself but more to test the behavior related to metric_for_best_model.

I'm not sure if actually running training would be necessary, though. Case 3 is simply to check whether a ValueError is being properly thrown at Trainer initialization time, and case 4 is also simply to check whether the __post_init__ method of TrainingArguments properly initializes metric_for_best_model to "loss" when save_strategy != "best" and load_best_model_at_end = True. To me, neither of these seem to require training/evaluation and Trainer instantiation seems sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, I would also split it into a separate test (or two test). And, yes, we are testing the init here, that's why it was looking out of place.

Copy link
Member

Choose a reason for hiding this comment

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

no strong opinion. We can split it into a separate test for case 3 and 4.

trainer = get_regression_trainer(
a=1.5,
b=2.5,
output_dir=tmpdir,
learning_rate=0.1,
eval_strategy="steps",
save_strategy="steps",
load_best_model_at_end=True,
)
self.assertTrue(trainer.args.metric_for_best_model == "loss")


@require_torch
@is_staging_test
Expand Down