Pipeline to device #210

pcuenca · 2022-08-18T13:01:28Z

The following will run the pipeline in cuda:1 (not cuda:0) as would be expected:

pipe = StableDiffusionPipeline.from_pretrained(
    "CompVis/stable-diffusion-v1-3-diffusers",
    use_auth_token=True).to("cuda:1")
pipe("Some prompt")

However, passing a device to __call__ will still move the pipeline to that device.

The implementation of DiffusionPipeline.to() does nothing if the device is None. This is to preserve the same semantics as PyTorch, where AFAIK if you use to(None) the object is not moved anywhere.

If we were to forgo that behaviour, we could make DiffusionPipeline.to(None) select cuda by default (when available). This would make for much simpler code in all pipeline implementations, as they'd just need to invoke self.to, but might break the expectations of PyTorch users. The current implementation in all the pipelines __call__ methods does exactly that: select cuda if available, and if no previous device was already set by the user. It is a bit repetitive and maybe a bit fragile.

Is it important to preserve PyTorch semantics in this regard, or is it better to make all __call__ implementations simpler? What do you think @anton-l @patil-suraj @patrickvonplaten ?

Fixes #195

Note that pipelines will still be moved to the default cuda device during `__call__` unless the same device is used there. Addressing that in a separate commit.

The following will run the pipeline in cuda:1 (not cuda:0) as expected: ```Python pipe = StableDiffusionPipeline.from_pretrained( "CompVis/stable-diffusion-v1-3-diffusers", use_auth_token=True).to("cuda:1") pipe("Some prompt") ``` I debated whether to place this logic in `DiffusionPipeline.to()`. It would make for much simpler code in all pipeline implementations (they just need to invoke `self.to`), but might break the expectations of PyTorch users, where AFAIK using `to(None)` does not move the object anywhere.

HuggingFaceDocBuilderDev · 2022-08-18T13:04:14Z

The documentation is not available anymore as the PR was closed or merged.

anton-l

Thanks for the PR @pcuenca!

Haven't seen cases of .to(None) in the wild before, so to me it's fine to break the expectations a bit :)
So implementing it like this:

def to(self, torch_device: Optional[Union[str, torch.device]] = None):
        if torch_device is None:
            torch_device = "cuda" if torch.cuda.is_available() else "cpu"
        ...

And then just doing self.to(torch_device) inside the pipelines would be much cleaner and less implementation error-prone for future pipelines

pcuenca · 2022-08-18T15:05:52Z

Haven't seen cases of .to(None) in the wild before, so to me it's fine to break the expectations a bit :)

Cool, I agree! That's how I would have done it if it had been for an internal project. Because it's open source, I might have been extremely cautious here :)

def to(self, torch_device: Optional[Union[str, torch.device]] = None):
        if torch_device is None:
            torch_device = "cuda" if torch.cuda.is_available() else "cpu"
        ...
And then just doing self.to(torch_device) inside the pipelines would be much cleaner and less implementation error-prone for future pipelines

One thing, though. If we expect users to use .to(device) and then not provide any device during __call__, our implementation must avoid moving the models to the default cuda device if a previous one was selected.

I'll prepare it right away so you can take a look.

pcuenca · 2022-08-18T15:39:58Z

@anton-l not super happy after the change, .to() works as expected, but the __call__ methods need to be careful to use self.device instead of the argument. What do you think?

anton-l · 2022-08-18T16:22:01Z

Hmm, indeed. But choosing between the first version and this one, I would still go with the current image.to(self.device) 🙂
Guess there are just two options available, wdyt @patil-suraj?

patrickvonplaten

Throwing in an idea -> we could also just remove torch_device as in input to the forward function. It's more "PyTorch'y" to use the .to(...) API IMO.

So we could just deprecate the "torch_device" parameter, saying that it'll be removed in 0.3.0 and only rely on to(...).

What do you think?

pcuenca · 2022-08-18T18:38:05Z

we could also just remove torch_device as in input to the forward function.

I think that's a very sensible idea. But I also love that if you do nothing it woks as expected (uses cuda if available), so we'd do an automatic placement on creation. Does that sound right?

patil-suraj

Thanks a lot for the PR @pcuenca !

I agree with @patrickvonplaten about removing the torch_device argument. Would be nice to have just one API for device handling, rather than having both options. That would be much cleaner IMO.

src/diffusers/pipeline_utils.py

patrickvonplaten · 2022-08-18T20:01:44Z

torch_device

I'd actually say to not do an automatic displacement to really stay 1-to-1 the same as PyTorch. I really like the fact that you know in PyTorch models are always on "cpu" by default.

@anton-l @patil-suraj what do you think?

pcuenca · 2022-08-18T23:13:09Z

I'd actually say to not do an automatic displacement to really stay 1-to-1 the same as PyTorch. I really like the fact that you know in PyTorch models are always on "cpu" by default.

That's a breaking change with respect to the current version. No big deal, and it's easy to understand in terms of PyTorch resemblance. I personally liked that you had to do nothing and the pipeline selected the GPU by default; to me, the pipeline is a high-level solution that is there to help you by providing sensible defaults. Similar to disabling gradients computation.

But the alternatives are a bit feeble, so this might well be the best compromise.

patrickvonplaten · 2022-08-19T07:53:39Z

+1 it is indeed a bigger breaking change.

For me, one of the two options sounds like the best:

1.)
Remove torch_device from forward(...) and keep defaulting the init on GPU. Thinking more about it I'm actually fine with it! Also, considering that when passing device="auto" to from_pretrained(...) of Transformers the model is automatically moved to GPU. I would then maybe add a logging statement though (pipeline moved on GPU...).
However the drawback of this approach is that what do we do when multiple GPUs are available? device="auto" in this case moves different layers on different GPUs - that's something that is out of scope here IMO, so we would probably just move it on the first GPU

2.)
Remove torch_device from forward(...) and change default init to CPU. Big advantage that we leave the complexity of possible multi-device placement up to the user & it's more PyTorchy.
Drawback: More of a breaking change, but I think it's fine at that stage of the library.

@patil-suraj @anton-l @pcuenca - let's maybe try to decide somewhat quickly now so that we can include this PR in today's release?

anton-l · 2022-08-19T08:26:18Z

I'm in favor of option 1 out of what Patrick suggested. "auto" can be improved a bit later, while defaulting to "cpu" might increase friction for first-timers

patrickvonplaten · 2022-08-19T08:47:12Z

Thinking a bit more about it, I'm actually much more for option 2, mainly because:

Don't want to handle the complexity of device placement in __init__.py
People know how PyTorch works -> don't think there is much friction
Transformer's pipelines also place on CPU by default: https://github.com/huggingface/transformers/blob/e54a1b49aa6268c484625c6374f952f318914743/src/transformers/pipelines/base.py#L750
device_map="auto" is not and will not become a default in transformers and we won't have it in the near future in diffusers I think (it's mostly there because transformers models are huge)

Happy to adhere to 1. though if @patil-suraj @anton-l and @pcuenca you prefer

pcuenca · 2022-08-19T08:52:29Z

Those are great points, @patrickvonplaten, let's go for the simpler option and do 2 instead.

anton-l · 2022-08-19T08:56:44Z

Ok, good points, option 2 it is! :)

patil-suraj · 2022-08-19T09:05:39Z

I'm also very much in favor of 2. with .to we trying to be more pytorchy, so we should try to mimic as close as possible to avoid any confusion. Let the user handle everything related to device.

`pipeline.to()` now has PyTorch semantics.

pcuenca · 2022-08-19T09:58:22Z

I did 2, can you please take another look? @patil-suraj @anton-l @patrickvonplaten

We possibly need to change some documentation and examples. Should we do that in a separate PR?

src/diffusers/pipelines/ddim/pipeline_ddim.py

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>

patil-suraj

Looking good! Pretty much the same comment as @anton-l and @patrickvonplaten .

Also think thekwargs should go in all pipelines.

src/diffusers/pipelines/ddim/pipeline_ddim.py

patil-suraj · 2022-08-19T13:04:18Z

src/diffusers/pipelines/ddim/pipeline_ddim.py

+                "`torch_device` is deprecated as an input argument to `__call__` and will be removed in v0.3.0.
+                Consider using `pipe.to(torch_device)` instead."
+           )
+        # ...set device as previously 


(nit) this comment should go above the if cond

anton-l · 2022-08-19T13:20:35Z

Yes, kwargs should be supported in all updated pipelines @pcuenca, sorry for commenting only on DDIM :)

anton-l

Looks great!

anton-l · 2022-08-19T14:10:29Z

Ok, let's resolve the conflicts and merge if @patrickvonplaten and @patil-suraj don't have any objections :)

patrickvonplaten

Looks good to me! @anton-l feel free to merge!

pcuenca · 2022-08-19T15:52:05Z

Yes, kwargs should be supported in all updated pipelines @pcuenca, sorry for commenting only on DDIM :)

Sure, no problem at all, that's what I understood :)

pcuenca · 2022-08-19T15:55:08Z

Sorry, I have family business going on and can't fix the conflicts right now. Can you do it @anton-l ?, otherwise I'll do it later.

* Implement `pipeline.to(device)` * DiffusionPipeline.to() decides best device on None. * Breaking change: torch_device removed from __call__ `pipeline.to()` now has PyTorch semantics. * Use kwargs and deprecation notice Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> * Apply torch_device compatibility to all pipelines. * style Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> Co-authored-by: anton-l <anton@huggingface.co>

The PyTorch decomposition for the op `aten.upsample_bilinear2d.vec` is merged in the upstream repo and hence removed from this file.

* Implement `pipeline.to(device)` * DiffusionPipeline.to() decides best device on None. * Breaking change: torch_device removed from __call__ `pipeline.to()` now has PyTorch semantics. * Use kwargs and deprecation notice Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> * Apply torch_device compatibility to all pipelines. * style Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> Co-authored-by: anton-l <anton@huggingface.co>

pcuenca added 2 commits August 18, 2022 14:29

Implement pipeline.to(device)

7ad7227

Note that pipelines will still be moved to the default cuda device during `__call__` unless the same device is used there. Addressing that in a separate commit.

anton-l requested changes Aug 18, 2022

View reviewed changes

DiffusionPipeline.to() decides best device on None.

f4e691c

anton-l requested review from patrickvonplaten and patil-suraj August 18, 2022 16:43

patrickvonplaten reviewed Aug 18, 2022

View reviewed changes

patil-suraj reviewed Aug 18, 2022

View reviewed changes

src/diffusers/pipeline_utils.py Outdated Show resolved Hide resolved

src/diffusers/pipeline_utils.py Outdated Show resolved Hide resolved

src/diffusers/pipeline_utils.py Outdated Show resolved Hide resolved

Breaking change: torch_device removed from __call__

bc5d7e3

`pipeline.to()` now has PyTorch semantics.

anton-l reviewed Aug 19, 2022

View reviewed changes

src/diffusers/pipelines/ddim/pipeline_ddim.py Outdated Show resolved Hide resolved

Use kwargs and deprecation notice

71635b2

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>

patil-suraj reviewed Aug 19, 2022

View reviewed changes

Apply torch_device compatibility to all pipelines.

a3129a1

anton-l approved these changes Aug 19, 2022

View reviewed changes

patrickvonplaten approved these changes Aug 19, 2022

View reviewed changes

anton-l added 2 commits August 19, 2022 18:34

Resolve merge conflicts

ffb5a61

style

f287c4c

anton-l merged commit 71ba8ae into main Aug 19, 2022

patil-suraj deleted the pipeline-to-device branch August 19, 2022 16:56

Pipeline to device #210

Pipeline to device #210

Uh oh!

Conversation

pcuenca commented Aug 18, 2022

Uh oh!

HuggingFaceDocBuilderDev commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anton-l left a comment

Choose a reason for hiding this comment

Uh oh!

pcuenca commented Aug 18, 2022

Uh oh!

pcuenca commented Aug 18, 2022

Uh oh!

anton-l commented Aug 18, 2022

Uh oh!

patrickvonplaten left a comment

Choose a reason for hiding this comment

Uh oh!

pcuenca commented Aug 18, 2022

Uh oh!

patil-suraj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrickvonplaten commented Aug 18, 2022

Uh oh!

pcuenca commented Aug 18, 2022

Uh oh!

patrickvonplaten commented Aug 19, 2022

Uh oh!

anton-l commented Aug 19, 2022

Uh oh!

patrickvonplaten commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pcuenca commented Aug 19, 2022

Uh oh!

anton-l commented Aug 19, 2022

Uh oh!

patil-suraj commented Aug 19, 2022

Uh oh!

pcuenca commented Aug 19, 2022

Uh oh!

Uh oh!

patil-suraj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patil-suraj Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

anton-l commented Aug 19, 2022

Uh oh!

anton-l left a comment

Choose a reason for hiding this comment

Uh oh!

anton-l commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickvonplaten left a comment

Choose a reason for hiding this comment

Uh oh!

pcuenca commented Aug 19, 2022

Uh oh!

pcuenca commented Aug 19, 2022

Uh oh!

Uh oh!

HuggingFaceDocBuilderDev commented Aug 18, 2022 •

edited

Loading

patrickvonplaten commented Aug 19, 2022 •

edited

Loading

anton-l commented Aug 19, 2022 •

edited

Loading