Fix return_dict in encodec #31646
Fix return_dict in encodec #31646
Conversation
There was a problem hiding this comment.
Thanks @jla524 for iterating on this!
I left a comment for small suggested changes :)
| if return_dict is None: | ||
| return_dict = self.config.return_dict |
There was a problem hiding this comment.
Here we could do instead:
| if return_dict is None: | |
| return_dict = self.config.return_dict | |
| return_dict = return_dict if return_dict is not None else self.config.return_dict |
so that return_dict is still set to self.config.return_dict by default.
There was a problem hiding this comment.
It would be also nice to do the same modification in the decode method (at this line).
|
Thanks for iterating on this @jla524! @amyeroberts this should be ready for final review/merge :) |
|
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. |
| if isinstance(tuple_object, (List, Tuple)): | ||
| for tuple_iterable_value, dict_iterable_value in zip(tuple_object, dict_object): | ||
| recursive_check(tuple_iterable_value, dict_iterable_value) | ||
| elif isinstance(tuple_object, Dict): |
There was a problem hiding this comment.
Even if we assert that tuple_object is a tuple on L377, as this is a recursive function, isn't is still possible that the values in tuple_object i.e. tuple_iterable_value are a dict or None?
There was a problem hiding this comment.
The values in the tuple_object should be tensor.Tensor only.
To check the types, I added a print statement in the recursive function:
def recursive_check(tuple_object, dict_object):
print(f"[DEBUG]: {type(tuple_object)=}, {type(dict_object)=}")
...and I got this:
[DEBUG]: type(tuple_object)=<class 'tuple'>, type(dict_object)=<class 'transformers.models.encodec.modeling_encodec.EncodecOutput'>
[DEBUG]: type(tuple_object)=<class 'torch.Tensor'>, type(dict_object)=<class 'torch.Tensor'>
[DEBUG]: type(tuple_object)=<class 'torch.Tensor'>, type(dict_object)=<class 'torch.Tensor'>edit: it's probably more intuitive to just iterate over the items and compare it
What does this PR do?
Fixes #31642 (issue)
With this PR,
return_dict=Falsereturns a tuple, and the unit test compares tuple vs dict values.Who can review?
@kamilakesbi