Allow trainer to return eval. loss for CLIP-like models #20214

ydshieh · 2022-11-14T16:53:51Z

What does this PR do?

Allow trainer to give evaluation loss for CLIP-like models.

Currently, this line

transformers/src/transformers/trainer.py

Line 3192 in 07d8d6e

    
           has_labels = False if len(self.label_names) == 0 else all(inputs.get(k) is not None for k in self.label_names)

gives has_labels = False for CLIP-like models, and can't give loss value in the evaluation.

without this PR:

***** eval metrics *****
  epoch                   =        1.0
  eval_runtime            = 0:00:01.67
  eval_samples_per_second =      9.571
  eval_steps_per_second   =      4.785

with this PR.

***** eval metrics *****
  epoch                   =        1.0
  eval_loss               =     0.8159
  eval_runtime            = 0:00:01.66
  eval_samples_per_second =      9.598
  eval_steps_per_second   =      4.799

HuggingFaceDocBuilder · 2022-11-14T17:07:40Z

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

ydshieh · 2022-11-14T20:42:21Z

src/transformers/trainer.py

        """
        has_labels = False if len(self.label_names) == 0 else all(inputs.get(k) is not None for k in self.label_names)
+        # For CLIP-like models capable of returning loss values.
+        can_compute_loss = True if len(self.label_names) == 0 and self.can_return_loss else False


We need to restrict to len(self.label_names) == 0.

For models that has len(self.label_names) > 0, we should check if the inputs contain the required labels by the model - which is done one line above for has_labels.

can_compute_loss actually means can_compute_loss_without_labels, but maybe a too long name.

Can we rename all can_compute_loss to loss_without_labels then? It's more informative even if not completely perfect.

sgugger

Very nice idea thanks a lot! I left a few comments regarding naming and we can make the function can_return_loss a bit better, but overall great PR!

sgugger · 2022-11-15T14:07:29Z

src/transformers/trainer.py

        """
        has_labels = False if len(self.label_names) == 0 else all(inputs.get(k) is not None for k in self.label_names)
+        # For CLIP-like models capable of returning loss values.
+        can_compute_loss = True if len(self.label_names) == 0 and self.can_return_loss else False


Can we rename all can_compute_loss to loss_without_labels then? It's more informative even if not completely perfect.

sgugger · 2022-11-15T14:08:59Z

src/transformers/utils/generic.py

+        signature = inspect.signature(model_class.__call__)
+    else:
+        signature = inspect.signature(model_class.forward)
+    return [p for p in signature.parameters if p == "return_loss"]


I'd prefer if this returns a bool then rely on Python magic conversion to bools.
Also I think we should check if the default is True as the Trainer won't change the default.

I changed it to

any(p == "return_loss" for p in signature.parameters)

but do you mean we should have sth. like (conceptually)

any(p == "return_loss" and default_value(p) is True for p in signature.parameters)

?

Yes to the second!

sgugger · 2022-11-15T15:10:55Z

src/transformers/trainer.py

        """
        has_labels = False if len(self.label_names) == 0 else all(inputs.get(k) is not None for k in self.label_names)
+        # For CLIP-like models capable of returning loss values.
+        loss_without_labels = True if len(self.label_names) == 0 and self.can_return_loss and inputs.get("return_loss", None) is True else False


inputs will not contain return_loss if True is the default.

OK! I see all of our return_loss have None as default value (include CLIP), but I can add extra check to be sure

Oh in this case then your solution works.

ydshieh · 2022-11-15T16:32:33Z

src/transformers/trainer.py

+        # If `return_loss` is not specified or being `None` in `inputs`, we check if the default value of `return_loss`
+        # is `True` in `model.forward`.
+        return_loss = inputs.get("return_loss", None)
+        if return_loss is None:


If False, no need to check default value

ydshieh · 2022-11-15T16:36:28Z

src/transformers/utils/generic.py

+
+    for p in signature.parameters:
+        if p == "return_loss" and signature.parameters[p].default is True:
+            return True


Will return True only if the default value is True.

ydshieh · 2022-11-15T16:37:32Z

@sgugger Hopefully the change covers everything that could happen now and in the future.

sgugger

Perfect!

…20214) * Allow trainer to return loss for CLIP-like models * Apply suggestions * update * update * update Co-authored-by: ydshieh <[email protected]>

Allow trainer to return loss for CLIP-like models

3655431

ydshieh requested a review from sgugger November 14, 2022 17:35

ydshieh changed the title ~~Allow trainer to return loss for CLIP-like models~~ Allow trainer to return eval. loss for CLIP-like models Nov 14, 2022

ydshieh commented Nov 14, 2022

View reviewed changes

sgugger reviewed Nov 15, 2022

View reviewed changes

ydshieh added 2 commits November 15, 2022 15:25

Apply suggestions

e34a59d

update

38e38f1

sgugger reviewed Nov 15, 2022

View reviewed changes

update

159180d

ydshieh commented Nov 15, 2022

View reviewed changes

update

944d997

ydshieh commented Nov 15, 2022

View reviewed changes

sgugger approved these changes Nov 15, 2022

View reviewed changes

ydshieh merged commit 0d0d776 into main Nov 15, 2022

ydshieh deleted the clip_loss branch November 15, 2022 18:47

Allow trainer to return eval. loss for CLIP-like models #20214

Allow trainer to return eval. loss for CLIP-like models #20214

Uh oh!

Conversation

ydshieh commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Uh oh!

HuggingFaceDocBuilder commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ydshieh Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ydshieh Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

ydshieh commented Nov 15, 2022

Uh oh!

sgugger left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ydshieh commented Nov 14, 2022 •

edited

Loading

HuggingFaceDocBuilder commented Nov 14, 2022 •

edited

Loading

ydshieh Nov 14, 2022 •

edited

Loading

ydshieh Nov 14, 2022 •

edited

Loading