Ensure RewardConfig is backwards compatible
#748
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
| " It will be set to `512` by default, but you should do it yourself in the future.", | ||
| UserWarning, | ||
| ) | ||
| max_length = 512 |
There was a problem hiding this comment.
Side remark for a later PR, but I think this should ideally be set to be the model's max context size as the default
| "You cannot specify both `max_length` and `args.max_length`. Please use the `RewardConfig` to set `max_length` once." | ||
| ) | ||
| if max_length is not None and args.max_length is None: | ||
| if type(args) == TrainingArguments: |
There was a problem hiding this comment.
| if type(args) == TrainingArguments: | |
| if isinstance(args, TrainingArguments): |
There was a problem hiding this comment.
I tried this initially, but realised it wont' work because args is a subclass of TrainingArguments and thus is an instance of TrainingArguments which makes the if-statement always true. See e.g. this: https://stackoverflow.com/questions/1549801/what-are-the-differences-between-type-and-isinstance
Happy to refactor to a try / except clause if you prefer :)
There was a problem hiding this comment.
I see perfect, thanks a lot lewis for explaining!
| max_length = 512 | ||
| if max_length is None and args.max_length is not None: | ||
| max_length = args.max_length | ||
| if type(args) == TrainingArguments: |
There was a problem hiding this comment.
| if type(args) == TrainingArguments: | |
| if isinstance(args, TrainingArguments): |
In #726 we replaced the
RewardTrainer.argswith a dedicatedRewardConfigto collect all hyperparameters in a dedicated class (in particularmax_length).Unfortunately, that implementation wasn't backwards compatible because
RewardTrainer.argswas of typetransformers.TrainingArgumentsand that object doesn't have themax_lengthattribute. This meant that part of the logic which checks for the presence ofargs.max_lengthwould throw an error for any users who hadn't updated their code to useRewardConfig.This PR fixes that by first checking the type of
RewardTrainer.argsand raising a warning if it'stransformers.TrainingArguments.Happy to add a unit test for this, but I felt it was a bit of overkill.