Skip to content

Conversation

@cbensimon
Copy link
Contributor

@cbensimon cbensimon commented Sep 29, 2023

Unpickling pipeline outputs currently produces an error (because the output is both a @dataclass and an OrderedDict)
This PR fixes this by setting a custom __reduce__ method on the BaseOutput class

@cbensimon cbensimon changed the title Make BaseOutput dataclasses picklable Make BaseOutput dataclasses picklable Sep 29, 2023
@cbensimon cbensimon marked this pull request as ready for review September 29, 2023 11:01
# Don't call self.__setattr__ to avoid recursion errors
super().__setattr__(key, value)

def __reduce__(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works! In Python, the __reduce__ function is more or less exclusively reserved for pickle no? So there should be no other side-effects that could be trigger here?

Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok for me! I don't think Transformer's model output class has this reduce feature either (see here: https://github.com/huggingface/transformers/blob/391177441b133645c02181b57370ab12f71b88c4/src/transformers/utils/generic.py#L288) but I don't see a reason why we should not add it

@patrickvonplaten
Copy link
Contributor

cc @LysandreJik for visibility in case it's needed for Transformers

@patrickvonplaten patrickvonplaten merged commit 9cfd4ef into main Sep 29, 2023
@patrickvonplaten patrickvonplaten deleted the base-output-reduce branch September 29, 2023 14:35
@cbensimon
Copy link
Contributor Author

Ok for me! I don't think Transformer's model output class has this reduce feature either (see here: https://github.com/huggingface/transformers/blob/391177441b133645c02181b57370ab12f71b88c4/src/transformers/utils/generic.py#L288) but I don't see a reason why we should not add it

Good catch, I wasn't aware that transformers were also using this dataclass OrderedDict class.
Yes I agree that there's no reason to not make ModelOutput serializable in transformers too

cbensimon added a commit to cbensimon/transformers that referenced this pull request Sep 29, 2023
@cbensimon
Copy link
Contributor Author

Just opened a draft PR in transformers : huggingface/transformers#26493

chuzhdontcode pushed a commit to chuzhdontcode/diffusers that referenced this pull request Oct 4, 2023
* Make BaseOutput dataclasses picklable

* make style

* Test

* Empty commit

* Simpler and safer
ydshieh pushed a commit to huggingface/transformers that referenced this pull request Oct 5, 2023
* Make `ModelOutput` serializable

Original PR from diffusers : huggingface/diffusers#5234

* Black
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
* Make BaseOutput dataclasses picklable

* make style

* Test

* Empty commit

* Simpler and safer
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
* Make BaseOutput dataclasses picklable

* make style

* Test

* Empty commit

* Simpler and safer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants