⚠️ Add warning guidelines and update codebase to follow best practices #2350

qgallouedec · 2024-11-11T23:43:14Z

What does this PR do?

Some warnings, previously triggered during normal operation, have been removed. As a consequence it reduces noise in our CI pipeline. This change brings us closer to our goal of zero warnings.

CI Warnings Count (Dev Dependencies):

Before PR: 558
After PR: 466

A follow-up PR is planned to address the remaining legitimate warnings.

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? 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.
Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

qgallouedec · 2024-11-11T23:44:17Z

trl/trainer/bco_trainer.py

-            warnings.warn(
-                "You passed a model_id to the BCOTrainer. This will automatically create an "
-                "`AutoModelForCausalLM` or a `PeftModel` (if you passed a `peft_config`) for you."
-            )


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-11T23:44:23Z

trl/trainer/bco_trainer.py

-            warnings.warn(
-                "You passed a ref model_id to the BCOTrainer. This will automatically create an "
-                "`AutoModelForCausalLM`"
-            )


Warnings should not indicate normal behavior.

HuggingFaceDocBuilderDev · 2024-11-11T23:46:54Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

qgallouedec · 2024-11-11T23:56:53Z

examples/scripts/reward_modeling.py

@@ -99,7 +99,8 @@
    if model_config.use_peft and model_config.lora_task_type != "SEQ_CLS":
        warnings.warn(
            "You are using a `task_type` that is different than `SEQ_CLS` for PEFT. This will lead to silent bugs"
-            " Make sure to pass --lora_task_type SEQ_CLS when using this script with PEFT."
+            " Make sure to pass --lora_task_type SEQ_CLS when using this script with PEFT.",
+            UserWarning,


Use the appropriate warning type.

qgallouedec · 2024-11-11T23:57:08Z

trl/core.py

@@ -296,7 +296,8 @@ def randn_tensor(
                warnings.warn(
                    f"The passed generator was created on 'cpu' even though a tensor on {device} was expected."
                    f" Tensors will be created on 'cpu' and then moved to {device}. Note that one can probably"
-                    f" slighly speed up this function by passing a generator that was created on the {device} device."
+                    f" slighly speed up this function by passing a generator that was created on the {device} device.",
+                    UserWarning,


Use the appropriate warning type.

qgallouedec · 2024-11-11T23:58:02Z

trl/trainer/utils.py

-    if np.array(predictions[:, 0] == predictions[:, 1], dtype=float).sum() > 0:
+    equal_predictions_count = np.array(predictions[:, 0] == predictions[:, 1], dtype=float).sum()
+    if equal_predictions_count > 0:
        warnings.warn(
-            f"There are {np.array(predictions[:, 0] == predictions[:, 1]).sum()} out of {len(predictions[:, 0])} instances where the predictions for both options are equal. As a consequence the accuracy can be misleading."
+            f"There are {equal_predictions_count} out of {len(predictions[:, 0])} instances where the predictions for "
+            "both options are equal. As a consequence the accuracy can be misleading.",
+            UserWarning,


Warnings must be actionable.
Warnings should not indicate normal behavior.
Use the appropriate warning type.

This warning remains not actionable. Any idea to solve this?

qgallouedec · 2024-11-12T15:33:15Z

trl/environment/base_environment.py

-            warnings.warn("install rich to display text")
-            return
+            raise ImportError(
+                "The `rich` library is required to display text with formatting. "
+                "Install it using `pip install rich`."
+            )


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T15:33:43Z

trl/environment/base_environment.py

-            warnings.warn("install rich to display tokens")
-            return
+            raise ImportError(
+                "The `rich` library is required to display tokens with formatting. "
+                "Install it using `pip install rich`."
+            )


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T15:33:52Z

trl/environment/base_environment.py

-            warnings.warn("install rich to display colour legend")
-            return
+            raise ImportError(
+                "The `rich` library is required to display colour legends with formatting. "
+                "Install it using `pip install rich`."
+            )


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T15:34:22Z

trl/models/modeling_sd_base.py

-                    "If you are aware that the pretrained model has no lora weights to it, ignore this message. "
-                    "Otherwise please check the if `pytorch_lora_weights.safetensors` exists in the model folder."
+                    "Trying to load LoRA weights but no LoRA weights found. Set `use_lora=False` or check that "
+                    "`pytorch_lora_weights.safetensors` exists in the model folder.",
+                    UserWarning,


Warnings must be actionable.
Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T15:40:44Z

trl/trainer/alignprop_config.py

-        if self.log_with not in ["wandb", "tensorboard"]:
-            warnings.warn(
-                "Accelerator tracking only supports image logging if `log_with` is set to 'wandb' or 'tensorboard'."
-            )


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T15:41:02Z

trl/trainer/alignprop_config.py

-        if self.log_with == "wandb" and not is_torchvision_available():
-            warnings.warn("Wandb image logging requires torchvision to be installed")


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T15:49:10Z

trl/trainer/bco_trainer.py

-                "You set `output_router_logits` to True in the model config, but `router_aux_loss_coef` is set to 0.0,"
-                " meaning the auxiliary loss will not be used."
+                "You set `output_router_logits` to `True` in the model config, but `router_aux_loss_coef` is set to "
+                "`0.0`, meaning the auxiliary loss will not be used. Either set `router_aux_loss_coef` to a value "
+                "greater than `0.0`, or set `output_router_logits` to `False` if you don't want to use the auxiliary "
+                "loss.",
+                UserWarning,


Warnings must be actionable.
Use the appropriate warning type.

qgallouedec · 2024-11-12T15:49:38Z

trl/trainer/bco_trainer.py

@@ -705,7 +700,6 @@ def make_inputs_require_grad(module, input, output):
        self.running = RunningMoments(accelerator=self.accelerator)

        if self.embedding_func is None:
-            warnings.warn("You did not pass `embedding_func` underlying distribution matching feature is deactivated.")


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T15:49:53Z

trl/trainer/bco_trainer.py

-        if not os.path.isfile(running_file):
-            warnings.warn(f"Missing file {running_file}. Will use a new running delta value for BCO loss calculation")
-        else:
+        if os.path.isfile(running_file):


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T15:50:03Z

trl/trainer/bco_trainer.py

-            if not os.path.isfile(running_file):
-                warnings.warn(f"Missing file {clf_file}. Will use a new UDM classifier for BCO loss calculation")
-            else:
+            if os.path.isfile(running_file):


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T15:50:18Z

trl/trainer/bco_trainer.py

-        if not self.use_dpo_data_collator:
-            warnings.warn(
-                "prediction_step is only implemented for DPODataCollatorWithPadding, and you passed a datacollator that is different than "
-                "DPODataCollatorWithPadding - you might see unexpected behavior. Alternatively, you can implement your own prediction_step method if you are using a custom data collator"
-            )


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T15:56:05Z

trl/trainer/cpo_trainer.py

-        if not self.use_dpo_data_collator:
-            warnings.warn(
-                "prediction_step is only implemented for DPODataCollatorWithPadding, and you passed a datacollator that is different than "
-                "DPODataCollatorWithPadding - you might see unexpected behavior. Alternatively, you can implement your own prediction_step method if you are using a custom data collator"
-            )


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T15:56:15Z

trl/trainer/cpo_trainer.py

-        if not self.use_dpo_data_collator:
-            warnings.warn(
-                "compute_loss is only implemented for DPODataCollatorWithPadding, and you passed a datacollator that is different than "
-                "DPODataCollatorWithPadding - you might see unexpected behavior. Alternatively, you can implement your own prediction_step method if you are using a custom data collator"
-            )
-


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T15:56:28Z

trl/trainer/cpo_trainer.py

-            if self.cpo_alpha > 0:
-                warnings.warn(
-                    "You are using CPO-SimPO method because you set a non-zero cpo_alpha. "
-                    "This will result in the CPO-SimPO method "
-                    "(https://github.com/fe1ixxu/CPO_SIMPO/tree/main). "
-                    "If you want to use a pure SimPO method, please set cpo_alpha to 0."
-                )


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T15:57:22Z

trl/trainer/cpo_trainer.py

-                "You set `output_router_logits` to True in the model config, but `router_aux_loss_coef` is set to 0.0,"
-                " meaning the auxiliary loss will not be used."
+                "You set `output_router_logits` to `True` in the model config, but `router_aux_loss_coef` is set to "
+                "`0.0`, meaning the auxiliary loss will not be used. Either set `router_aux_loss_coef` to a value "
+                "greater than `0.0`, or set `output_router_logits` to `False` if you don't want to use the auxiliary "
+                "loss.",
+                UserWarning,


Warnings must be actionable.
Use the appropriate warning type.

qgallouedec · 2024-11-12T21:44:51Z

trl/trainer/kto_trainer.py

-        if not self.use_dpo_data_collator:
-            warnings.warn(
-                "prediction_step is only implemented for DPODataCollatorWithPadding, and you passed a datacollator that is different than "
-                "DPODataCollatorWithPadding - you might see unexpected behavior. Alternatively, you can implement your own prediction_step method if you are using a custom data collator"
-            )


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T21:45:01Z

trl/trainer/online_dpo_trainer.py

-                "Ignoring `judge` and using `reward_model`."
+                "Ignoring `judge` and using `reward_model`.",
+                UserWarning,


Use the appropriate warning type.

qgallouedec · 2024-11-12T21:45:12Z

trl/trainer/orpo_trainer.py

-            warnings.warn(
-                "You passed a model_id to the ORPOTrainer. This will automatically create an "
-                "`AutoModelForCausalLM` or a `PeftModel` (if you passed a `peft_config`) for you."
-            )


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T21:45:24Z

trl/trainer/orpo_trainer.py

-                "You set `output_router_logits` to True in the model config, but `router_aux_loss_coef` is set to 0.0,"
-                " meaning the auxiliary loss will not be used."
+                "You set `output_router_logits` to `True` in the model config, but `router_aux_loss_coef` is set to "
+                "`0.0`, meaning the auxiliary loss will not be used. Either set `router_aux_loss_coef` to a value "
+                "greater than `0.0`, or set `output_router_logits` to `False` if you don't want to use the auxiliary "
+                "loss.",
+                UserWarning,


Warnings must be actionable.
Use the appropriate warning type.

qgallouedec · 2024-11-12T21:45:43Z

trl/trainer/orpo_trainer.py

-        if not self.use_dpo_data_collator:
-            warnings.warn(
-                "compute_loss is only implemented for DPODataCollatorWithPadding, and you passed a datacollator that is different than "
-                "DPODataCollatorWithPadding - you might see unexpected behavior. Alternatively, you can implement your own prediction_step method if you are using a custom data collator"
-            )


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T21:46:37Z

trl/trainer/reward_trainer.py

-        if type(args) is TrainingArguments:
-            warnings.warn(
-                "Using `transformers.TrainingArguments` for `args` is deprecated and will be removed in a future version. Please use `RewardConfig` instead.",
-                FutureWarning,


It has been deprecated for more than a year (#748)

qgallouedec · 2024-11-12T21:47:27Z

trl/trainer/sft_trainer.py

@@ -126,9 +126,7 @@ def __init__(
        formatting_func: Optional[Callable] = None,
    ):
        if args is None:
-            output_dir = "tmp_trainer"
-            warnings.warn(f"No `SFTConfig` passed, using `output_dir={output_dir}`.")


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T21:47:37Z

trl/trainer/reward_trainer.py

-        if not self.use_reward_data_collator:
-            warnings.warn(
-                "The current compute_loss is implemented for RewardDataCollatorWithPadding,"
-                " if you are using a custom data collator make sure you know what you are doing or"
-                " implement your own compute_loss method."
-            )


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T21:48:05Z

trl/trainer/reward_trainer.py

@@ -189,7 +173,7 @@ def __init__(
                    "A processing_class must be specified when using the default RewardDataCollatorWithPadding"
                )
            if max_length is None:
-                max_length = 512 if type(args) is TrainingArguments or args.max_length is None else args.max_length


it has been deprecated for more than a year (#748)

qgallouedec · 2024-11-12T21:48:20Z

trl/trainer/reward_trainer.py

-                            "please update to the latest version of peft to use `gradient_checkpointing_kwargs`."
+                            "please update to the latest version of peft to use `gradient_checkpointing_kwargs`.",
+                            UserWarning,


Use the appropriate warning type.

qgallouedec · 2024-11-12T21:49:13Z

trl/trainer/sft_trainer.py

-            warnings.warn(
-                "You passed a model_id to the SFTTrainer. This will automatically create an "
-                "`AutoModelForCausalLM` or a `PeftModel` (if you passed a `peft_config`) for you."
-            )


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T21:49:25Z

trl/trainer/sft_trainer.py

-            warnings.warn(
-                f"You didn't pass a `max_seq_length` argument to the SFTTrainer, this will default to {args.max_seq_length}"
-            )
-


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T21:49:59Z

trl/trainer/sft_trainer.py

-                "You passed a processing_class with `padding_side` not equal to `right` to the SFTTrainer. This might lead to some unexpected behaviour due to "
-                "overflow issues when training a model in half-precision. You might consider adding `processing_class.padding_side = 'right'` to your code."
+                "You passed a processing_class with `padding_side` not equal to `right` to the SFTTrainer. This might "
+                "lead to some unexpected behaviour due to overflow issues when training a model in half-precision. "
+                "You might consider adding `processing_class.padding_side = 'right'` to your code.",
+                UserWarning,


Warnings must be actionable.
Use the appropriate warning type.

This warning is still not actionable. Any idea?

qgallouedec · 2024-11-12T21:50:14Z

trl/trainer/sft_trainer.py

-                warnings.warn(
-                    "You passed `packing=True` to the SFTTrainer/SFTConfig, and you are training your model with `max_steps` strategy. The dataset will be iterated until the `max_steps` are reached."
-                )


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T21:50:33Z

trl/trainer/sft_trainer.py

-                    "You passed a dataset that is already processed (contains an `input_ids` field) together with a valid formatting function. Therefore `formatting_func` will be ignored."
+                    "You passed a dataset that is already processed (contains an `input_ids` field) together with a "
+                    "valid formatting function. Therefore `formatting_func` will be ignored. Either remove the "
+                    "`formatting_func` or pass a dataset that is not already processed.",
+                    UserWarning,


Warnings must be actionable.
Use the appropriate warning type.

qgallouedec · 2024-11-12T21:51:05Z

trl/trainer/sft_trainer.py

-                "You passed `remove_unused_columns=False` on a non-packed dataset. This might create some issues with the default collator and yield to errors. If you want to "
-                f"inspect dataset other columns (in this case {extra_columns}), you can subclass `DataCollatorForLanguageModeling` in case you used the default collator and create your own data collator in order to inspect the unused dataset columns."
+                "You passed `remove_unused_columns=False` on a non-packed dataset. This might create some issues with "
+                "the default collator and yield to errors. If you want to inspect dataset other columns (in this "
+                f"case {extra_columns}), you can subclass `DataCollatorForLanguageModeling` in case you used the "
+                "default collator and create your own data collator in order to inspect the unused dataset columns.",
+                UserWarning,


Use the appropriate warning type.

qgallouedec · 2024-11-12T21:51:49Z

trl/trainer/utils.py

-                "To avoid this, set the pad_token_id to a different value."
+                "To avoid this, set the pad_token_id to a different value.",
+                UserWarning,


Warnings should not indicate normal behavior.
Use the appropriate warning type.

This warning can still occur in normal behavior. Any idea?

qgallouedec · 2024-11-12T21:52:02Z

trl/trainer/utils.py

-                        f"Could not find response key `{self.response_template}` in the "
-                        f'following instance: {self.tokenizer.decode(batch["input_ids"][i])} '
-                        f"This instance will be ignored in loss calculation. "
-                        f"Note, if this happens often, consider increasing the `max_seq_length`."
+                        f"Could not find response key `{self.response_template}` in the following instance: "
+                        f"{self.tokenizer.decode(batch["input_ids"][i])}. This instance will be ignored in loss "
+                        "calculation. Note, if this happens often, consider increasing the `max_seq_length`.",
+                        UserWarning,


Warnings should not indicate normal behavior.
Use the appropriate warning type.

This warning can still occur in normal behavior. Any idea?

qgallouedec · 2024-11-12T21:52:59Z

trl/trainer/utils.py

-                        f"Could not find response key `{self.response_template}` in the "
-                        f'following instance: {self.tokenizer.decode(batch["input_ids"][i])} '
-                        f"This instance will be ignored in loss calculation. "
-                        f"Note, if this happens often, consider increasing the `max_seq_length`."
+                        f"Could not find response key `{self.response_template}` in the following instance: "
+                        f"{self.tokenizer.decode(batch["input_ids"][i])}. This instance will be ignored in loss "
+                        "calculation. Note, if this happens often, consider increasing the `max_seq_length`.",
+                        UserWarning,


Warnings should not indicate normal behavior.
Use the appropriate warning type.

This warning can still occur in normal behavior. Any idea?

qgallouedec · 2024-11-12T21:53:15Z

trl/trainer/utils.py

-
-        if tokenizer.eos_token_id is None:
-            warnings.warn(
-                "The passed tokenizer does not have an EOS token. We will use the passed eos_token_id instead which corresponds"
-                f" to {eos_token_id}. If this is not the correct EOS token, make sure to pass the correct eos_token_id."
-            )
-


Warnings should not indicate normal behavior.

qgallouedec · 2024-11-12T21:53:28Z

trl/trainer/utils.py

-                        f"Could not find instruction key `{self.instruction_template}` in the "
-                        f'following instance: {self.tokenizer.decode(batch["input_ids"][i])} '
-                        f"This instance will be ignored in loss calculation. "
-                        f"Note, if this happens often, consider increasing the `max_seq_length`."
+                        f"Could not find instruction key `{self.instruction_template}` in the following instance: "
+                        f"{self.tokenizer.decode(batch["input_ids"][i])}. This instance will be ignored in loss "
+                        "calculation. Note, if this happens often, consider increasing the `max_seq_length`.",
+                        UserWarning,


Warnings should not indicate normal behavior.
Use the appropriate warning type.

This warning can still occur in normal behavior. Any idea?

…rnings

qgallouedec added 2 commits November 11, 2024 23:40

Add guidelines for working with warnings in the codebase

350a66a

Remove unnecessary warnings and improve code initialization

53ba260

qgallouedec commented Nov 11, 2024

View reviewed changes

Fix warnings and improve accuracy calculation

c74421e

qgallouedec commented Nov 11, 2024

View reviewed changes

qgallouedec added 2 commits November 12, 2024 15:16

Add rich library dependency for text formatting

5f21517

Update LoRA weight loading warning message

dbcac07

qgallouedec commented Nov 12, 2024

View reviewed changes

Fix logging and import issues in AlignPropConfig

c273e1f

qgallouedec commented Nov 12, 2024

View reviewed changes

Fix warnings and improve code readability

0a970e7

qgallouedec commented Nov 12, 2024

View reviewed changes

qgallouedec added 2 commits November 12, 2024 15:55

Remove unused import statements

ef398f0

Refactor CPOTrainer class in cpo_trainer.py

f471589

qgallouedec commented Nov 12, 2024

View reviewed changes

qgallouedec and others added 3 commits November 12, 2024 21:55

Fix string formatting in DataCollatorForCompletionOnlyLM class

6897470

Merge branch 'main' into warnings

49f5460

Merge branch 'main' into warnings

230a486

qgallouedec mentioned this pull request Nov 19, 2024

Change logging level from warning to info for max_steps overriding num_train_epochs huggingface/transformers#34810

Merged

5 tasks

qgallouedec and others added 5 commits November 20, 2024 10:11

Merge branch 'main' into warnings

d68b69d

Merge branch 'main' into warnings

c5f8f13

Update SimPO loss parameters in CPOTrainer

dcdc6ca

Merge branch 'warnings' of https://github.com/huggingface/trl into wa…

7ecd6ec

…rnings

Merge branch 'main' into warnings

efdea04

		if self.log_with == "wandb" and not is_torchvision_available():
		warnings.warn("Wandb image logging requires torchvision to be installed")

⚠️ Add warning guidelines and update codebase to follow best practices #2350

Are you sure you want to change the base?

⚠️ Add warning guidelines and update codebase to follow best practices #2350

Conversation

qgallouedec commented Nov 11, 2024 • edited Loading

What does this PR do?

Before submitting

Who can review?

qgallouedec Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

qgallouedec Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

HuggingFaceDocBuilderDev commented Nov 11, 2024

qgallouedec Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

qgallouedec Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

qgallouedec Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

qgallouedec Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qgallouedec Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qgallouedec commented Nov 11, 2024 •

edited

Loading

qgallouedec Nov 11, 2024 •

edited

Loading

qgallouedec Nov 11, 2024 •

edited

Loading

qgallouedec Nov 11, 2024 •

edited

Loading

qgallouedec Nov 11, 2024 •

edited

Loading

qgallouedec Nov 11, 2024 •

edited

Loading

qgallouedec Nov 12, 2024 •

edited

Loading

qgallouedec Nov 12, 2024 •

edited

Loading