diff --git a/src/transformers/generation/configuration_utils.py b/src/transformers/generation/configuration_utils.py index f601a97959c6..a8eb3a9c9d68 100644 --- a/src/transformers/generation/configuration_utils.py +++ b/src/transformers/generation/configuration_utils.py @@ -62,6 +62,23 @@ from .logits_process import SynthIDTextWatermarkLogitsProcessor, WatermarkLogitsProcessor +def _should_warn(outer_attr: str, inner_attr: str, user_set_attributes: set | None) -> bool: + """Determine if we should raise a warning for the combination `outer_attr` and `inner_attr`, based on whether + they were provided explicitly, i.e. if they were in `user_set_attributes`. + For example, if `outer_attr="do_sample"`, the warnings should be suppressed for `inner_attr` flags (e.g. "top_p") that weren't + explicitly set by the caller. When `do_sample=False` is explicitly required by the user, values such as `top_p` inherited + from a model's `generation_config.json` are harmless when the user opts for greedy decoding. + """ + outer_sample_set = user_set_attributes is not None and outer_attr in user_set_attributes + inner_attr_set = user_set_attributes is not None and inner_attr in user_set_attributes + # We should warn only if both are explicitly set, none are set, or only the inner_attr is set while outer_attr is not + return ( + (outer_sample_set and inner_attr_set) + or (not outer_sample_set and not inner_attr_set) + or (inner_attr_set and not outer_sample_set) + ) + + class GenerationMode(ExplicitEnum): """ Possible generation modes, downstream of the [`~generation.GenerationMixin.generate`] method. @@ -350,6 +367,11 @@ class GenerationConfig(PushToHubMixin): _original_object_hash: int | None def __init__(self, **kwargs): + # Snapshot of the attributes the caller explicitly provided (before the `kwargs.pop(...)` calls below + # consume them). Used by `validate()` to restrict "minor issue" warnings to flags actually set by the user, + # as opposed to defaults inherited from a model's `generation_config.json`. + user_set_attributes = set(kwargs.keys()) + # Parameters that control the length of the output self.max_length = kwargs.pop("max_length", None) self.max_new_tokens = kwargs.pop("max_new_tokens", None) @@ -466,7 +488,7 @@ def __init__(self, **kwargs): ) # Validate the values of the attributes - self.validate() + self.validate(user_set_attributes=user_set_attributes) def __hash__(self): return hash(self.to_json_string(ignore_metadata=True)) @@ -587,7 +609,7 @@ def _get_default_generation_params() -> dict[str, Any]: "diversity_penalty": 0.0, } - def validate(self, strict=False): + def validate(self, strict=False, user_set_attributes: set[str] | None = None): """ Validates the values of the attributes of the [`GenerationConfig`] instance. Raises exceptions in the presence of parameterization that can be detected as incorrect from the configuration instance alone. @@ -597,6 +619,11 @@ def validate(self, strict=False): Args: strict (bool): If True, raise an exception for any issues found. If False, only log issues. + user_set_attributes (set[str], *optional*): Names of attributes the caller explicitly provided. When + supplied, "minor issue" warnings about conflicting flag combinations (e.g. sampling-only flags set + while `do_sample=False`) only fire if the conflicting flag is in this set -- avoiding noisy warnings + when the value was inherited from a model's default `generation_config.json`. When `None`, all set + attributes are considered user-set (backward-compatible behavior for direct `validate()` calls). """ minor_issues = {} # format: {attribute_name: issue_description} @@ -636,47 +663,79 @@ def validate(self, strict=False): # Note that we check `is not True` in purpose. Boolean fields can also be `None` so we # have to be explicit. Value of `None` is same as having `False`, i.e. the default value + if self.do_sample is not True: greedy_wrong_parameter_msg = ( - "`do_sample` is set not to set `True`. However, `{flag_name}` is set to `{flag_value}` -- this flag is only " - "used in sample-based generation modes. You should set `do_sample=True` or unset `{flag_name}`." + "`do_sample` is not set to `True`. However, `{flag_name}` is set to `{flag_value}` -- this flag is " + "only used in sample-based generation modes. You should set `do_sample=True` or unset `{flag_name}`." ) - if self.temperature is not None and self.temperature != 1.0: + + if ( + self.temperature is not None + and self.temperature != 1.0 + and _should_warn("do_sample", "temperature", user_set_attributes) + ): minor_issues["temperature"] = greedy_wrong_parameter_msg.format( flag_name="temperature", flag_value=self.temperature ) - if self.top_p is not None and self.top_p != 1.0: + if ( + self.top_p is not None + and self.top_p != 1.0 + and _should_warn("do_sample", "top_p", user_set_attributes) + ): minor_issues["top_p"] = greedy_wrong_parameter_msg.format(flag_name="top_p", flag_value=self.top_p) - if self.min_p is not None: + if self.min_p is not None and _should_warn("do_sample", "min_p", user_set_attributes): minor_issues["min_p"] = greedy_wrong_parameter_msg.format(flag_name="min_p", flag_value=self.min_p) - if self.top_h is not None: + if self.top_h is not None and _should_warn("do_sample", "top_h", user_set_attributes): minor_issues["top_h"] = greedy_wrong_parameter_msg.format(flag_name="top_h", flag_value=self.top_h) - if self.typical_p is not None and self.typical_p != 1.0: + if ( + self.typical_p is not None + and self.typical_p != 1.0 + and _should_warn("do_sample", "typical_p", user_set_attributes) + ): minor_issues["typical_p"] = greedy_wrong_parameter_msg.format( flag_name="typical_p", flag_value=self.typical_p ) - if self.top_k is not None and self.top_k != 50: + if self.top_k is not None and self.top_k != 50 and _should_warn("do_sample", "top_k", user_set_attributes): minor_issues["top_k"] = greedy_wrong_parameter_msg.format(flag_name="top_k", flag_value=self.top_k) - if self.epsilon_cutoff is not None and self.epsilon_cutoff != 0.0: + if ( + self.epsilon_cutoff is not None + and self.epsilon_cutoff != 0.0 + and _should_warn("do_sample", "epsilon_cutoff", user_set_attributes) + ): minor_issues["epsilon_cutoff"] = greedy_wrong_parameter_msg.format( flag_name="epsilon_cutoff", flag_value=self.epsilon_cutoff ) - if self.eta_cutoff is not None and self.eta_cutoff != 0.0: + if ( + self.eta_cutoff is not None + and self.eta_cutoff != 0.0 + and _should_warn("do_sample", "eta_cutoff", user_set_attributes) + ): minor_issues["eta_cutoff"] = greedy_wrong_parameter_msg.format( flag_name="eta_cutoff", flag_value=self.eta_cutoff ) - # 2.2. detect beam-only parameterization when not in beam mode + # 2.2. detect beam-only parameterization when not in beam mode. Same provenance filtering as above -- + # both `num_beams` and the beam-only flag must be user-set for the warning to fire. if self.num_beams is None or self.num_beams == 1: single_beam_wrong_parameter_msg = ( - "`num_beams` is set to {num_beams}. However, `{flag_name}` is set to `{flag_value}` -- this flag is only used " - "in beam-based generation modes. You should set `num_beams>1` or unset `{flag_name}`." + "`num_beams` is set to {num_beams}. However, `{flag_name}` is set to `{flag_value}` -- this flag is " + "only used in beam-based generation modes. You should set `num_beams>1` or unset `{flag_name}`." ) - if self.early_stopping is not None and self.early_stopping is not False: + + if ( + self.early_stopping is not None + and self.early_stopping is not False + and _should_warn("num_beams", "early_stopping", user_set_attributes) + ): minor_issues["early_stopping"] = single_beam_wrong_parameter_msg.format( num_beams=self.num_beams, flag_name="early_stopping", flag_value=self.early_stopping ) - if self.length_penalty is not None and self.length_penalty != 1.0: + if ( + self.length_penalty is not None + and self.length_penalty != 1.0 + and _should_warn("num_beams", "length_penalty", user_set_attributes) + ): minor_issues["length_penalty"] = single_beam_wrong_parameter_msg.format( num_beams=self.num_beams, flag_name="length_penalty", flag_value=self.length_penalty ) @@ -1232,8 +1291,9 @@ def update(self, defaults_only=False, allow_custom_entries=False, **kwargs): setattr(self, key, value) to_remove.append(key) - # Confirm that the updated instance is still valid - self.validate() + # Confirm that the updated instance is still valid. Only attributes *explicitly* updated in this call count + # as user-set for warning purposes: defaults inherited from a model's config shouldn't emit warnings. + self.validate(user_set_attributes=set(to_remove)) # Remove all the attributes that were updated, without modifying the input dict unused_kwargs = {key: value for key, value in kwargs.items() if key not in to_remove} diff --git a/tests/generation/test_configuration_utils.py b/tests/generation/test_configuration_utils.py index 3ca904db0c57..36ddf4844d54 100644 --- a/tests/generation/test_configuration_utils.py +++ b/tests/generation/test_configuration_utils.py @@ -157,31 +157,47 @@ def test_validate(self): GenerationConfig() self.assertEqual(len(captured_logs.out), 0) - # Inconsequent but technically wrong configuration will throw a warning (e.g. setting sampling - # parameters with `do_sample=False`). May be escalated to an error in the future. + # Inconsequent but technically wrong configuration will throw a warning (e.g. requesting an extra output + # without `return_dict_in_generate=True`). May be escalated to an error in the future. logger.warning_once.cache_clear() with CaptureLogger(logger) as captured_logs: GenerationConfig(return_dict_in_generate=False, output_scores=True) self.assertNotEqual(len(captured_logs.out), 0) + # Explicitly setting a sampling flag alongside `do_sample=False` still warns: this is a user-level mistake. logger.warning_once.cache_clear() with CaptureLogger(logger) as captured_logs: generation_config_bad_temperature = GenerationConfig(do_sample=False, temperature=0.5) # store for later self.assertNotEqual(len(captured_logs.out), 0) - # Expanding on the case above, we can update a bad configuration to get rid of the warning. Ideally, - # that is done by unsetting the parameter (i.e. setting it to None) + # But a value inherited from a model's default config (i.e. not in this update's kwargs) does NOT warn: in + # the real world, `generate(do_sample=False)` on a model whose `generation_config.json` has `temperature=0.6` + # would otherwise log a useless warning. + logger.warning_once.cache_clear() + base_config = GenerationConfig(do_sample=True, temperature=0.6) # mimics a model's default config + with CaptureLogger(logger) as captured_logs: + base_config.update(do_sample=False) + self.assertEqual(len(captured_logs.out), 0) + + # Inverse provenance case: `do_sample=False` inherited from a model's config (so not user-set this call), user only + # sets a sampling flag. The conflict SHOULD produce noise because the user may think that it's non-greedy by default + logger.warning_once.cache_clear() + greedy_hub_config = GenerationConfig(do_sample=False) # mimics a model's default config forcing greedy + with CaptureLogger(logger) as captured_logs: + greedy_hub_config.update(top_p=0.8) + self.assertNotEqual(len(captured_logs.out), 0) + + # Updating only `temperature` (do_sample was pre-existing, i.e. "from the hub") does warn logger.warning_once.cache_clear() with CaptureLogger(logger) as captured_logs: - # BAD - 0.9 means it is still set, we should warn generation_config_bad_temperature.update(temperature=0.9) self.assertNotEqual(len(captured_logs.out), 0) + # But setting both in the same `update()` call DOES warn. logger.warning_once.cache_clear() with CaptureLogger(logger) as captured_logs: - # CORNER CASE - 1.0 is the default, we can't detect whether it is set by the user or not, we shouldn't warn - generation_config_bad_temperature.update(temperature=1.0) - self.assertEqual(len(captured_logs.out), 0) + generation_config_bad_temperature.update(do_sample=False, temperature=0.9) + self.assertNotEqual(len(captured_logs.out), 0) logger.warning_once.cache_clear() with CaptureLogger(logger) as captured_logs: @@ -230,6 +246,63 @@ def test_validate(self): with self.assertRaises(ValueError): generation_config.validate(strict=True) + def test_validate_sampling_flag_provenance(self): + """ + Dedicated coverage for the provenance-aware warning rule on sampling-only flags: + we only warn when BOTH `do_sample=False` AND a conflicting sampling flag (e.g. `top_p`, `temperature`) + were explicitly provided by the caller in the same context, or none of the 2 were directly provided, or only + the sampling flag is provided along do_sample=False already existing. + """ + logger = transformers_logging.get_logger("transformers.generation.configuration_utils") + + def _warn_count(fn): + logger.warning_once.cache_clear() + with CaptureLogger(logger) as captured: + fn() + return len(captured.out) + + # 1. Hub config sets `temperature`, user does only `generate(do_sample=False)` -> NO warning. + # (Emulates: model whose `generation_config.json` carries `do_sample=True, temperature=0.6`, user + # explicitly asks for greedy decoding.) + def case_hub_temp_user_do_sample_only(): + cfg = GenerationConfig(do_sample=True, temperature=0.6) # stands in for the hub default + cfg.update(do_sample=False) + + self.assertEqual(_warn_count(case_hub_temp_user_do_sample_only), 0) + + # 2. User explicitly sets BOTH `do_sample=False` and `top_p=0.8` in the same call -> WARN. + self.assertNotEqual(_warn_count(lambda: GenerationConfig(do_sample=False, top_p=0.8)), 0) + + # 3. User explicitly sets only `do_sample=False` (no sampling flag) -> NO warning, even though + # attribute defaults (like `top_k=50`) may be present. + self.assertEqual(_warn_count(lambda: GenerationConfig(do_sample=False)), 0) + + # 4. Hub config forces greedy (`do_sample=False`), user sets only `top_p=0.8` -> warnings: + # do_sample` was inherited, but clashes with user-expressed intent, so flagging their `top_p` + def case_hub_greedy_user_top_p(): + cfg = GenerationConfig(do_sample=False) # stands in for the hub default + cfg.update(top_p=0.8) + + self.assertNotEqual(_warn_count(case_hub_greedy_user_top_p), 0) + + # 5. User sets `do_sample=False` and `temperature=0.5` via a single `update()` call -> WARN. + def case_update_both_sides(): + cfg = GenerationConfig() + cfg.update(do_sample=False, temperature=0.5) + + self.assertNotEqual(_warn_count(case_update_both_sides), 0) + + # 6. Same idea for beam flags: user only asks for `num_beams=1`, hub default has `length_penalty=0.8` + # -> NO warning. + def case_hub_length_penalty_user_num_beams_only(): + cfg = GenerationConfig(num_beams=4, length_penalty=0.8) # stands in for the hub default + cfg.update(num_beams=1) + + self.assertEqual(_warn_count(case_hub_length_penalty_user_num_beams_only), 0) + + # 7. User sets BOTH `num_beams=1` and `length_penalty=0.8` explicitly -> WARN. + self.assertNotEqual(_warn_count(lambda: GenerationConfig(num_beams=1, length_penalty=0.8)), 0) + def test_refuse_to_save(self): """Tests that we refuse to save a generation config that fails validation."""