The failing tests seem unrelated to my changes and all tests passed locally for me, is there something I'm missing here?

Hey @zucchini-nlp thanks! I’ve added extra validations for the remaining generation parameters.

I also validated bad_words_ids inside GenerationConfig.validate() , to test it before running the forward pass, not at the end of it (when its NoBadWordsLogitsProcessor is executed).

For now, I’ve left the _validate_arguments() method in NoBadWordsLogitsProcessor intact, I was considering removing it since it’s now redundant, but I’d like get your green light before proceeding; If approved I would apply the same approach for sequence_bias and adjust their tests accordingly :)

Thanks @zucchini-nlp ! I believe all generation parameters are validated now

Thank you @zucchini-nlp for reviewing, when adding tests I also made updates to some existing ones:

• So an older PR changed the sequence_bias format because it didn’t serialize properly, but the docstring and code still reference the dictionary format to users, which is confusing and since _convert_list_arguments_into_dict() in SequenceBiasLogitsProcessor is used for internal backwards compatibility, the _validate_arguments() should raise a ValueError if the input is not in the list format, right?

• I also adjusted the forced_bos_token_id and forced_eos_token_id and added missing get_generation_mode, the rest of parameters I validated in the config seem be already tested.

Finally, I’m not entirely sure about how to address bad_words_ids in the failing CI/CD tests, any guidance here would be appreciated, thank you again!

cc @ArthurZucker whenever you have a moment, I'd appreciate your review, I wanted to see those CI tests green 😅 but it seems unrelated. Thanks!

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.

Thanks for reviewing this @ArthurZucker!

• dataclasses don't inherently validate data, it won’t raise an error if passed an invalid data type, so we would need to implement the validation logic manually anyway, relies on passing default model configs via kwargs which makes fully converting it to a dataclass impractical.

• Since you mentioned using a tool, a much simpler solution would to use attrs and it’s easily scalable, but it adds an extra dependency (Gante advised against using Pydantic I assume because it significantly increases runtime).

• If we don't want to add dependencies, we can refactor the logic to a mapping approach which is scalable, something like this:

class _ConfigValidator:

    VALIDATORS = {
        "pos_int_arg": (lambda x: isinstance(x, int) and x >= 0,
            "`{arg}` must be a positive integer, but got {value}."),

        "probability_arg": (lambda x: isinstance(x, (int, float)) and 0.0 <= x <= 1.0,
            "`{arg}` must be a float between 0 and 1, but got {value}."),
          ... }

    CONFIG_TYPES = {
        "top_k": "pos_int_arg",
        "top_p": "probability_arg",
         ...}


class GenerationConfig:
    ....

    def validate(self):
        for arg, value in self.__dict__.items():
            if arg in _ConfigValidator.CONFIG_TYPES:
                key = _ConfigValidator.CONFIG_TYPES[arg]
                validator, error_message = _ConfigValidator.VALIDATORS[key]
                if value is not None and not validator(value):
                    raise ValueError(error_message.format(arg=arg, value=value))

_ConfigValidator would hold also more complex validation for consistency and keep GenerationConfig.validate() less inflated for any future generation arguments. Let me know what you think before I move forward :)

I really don't mind using pydantic, it's become quite important and we have so many configs that we need to validate! Do you mind drafting a PR for that? 🤗

Hey @ArthurZucker, apologies for the delayed response, I just opened a PR for it!

@Manalelaidouni this PR is orthogonal to the pydantic PR, correct? Or should we merge one before the other?

@gante they're indeed orthogonal this was the original plan, so I'll close this PR :)