[core] AutoencoderMixin to abstract common methods#12473
Conversation
|
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. |
dg845
left a comment
There was a problem hiding this comment.
LGTM! Do you think the remaining autoencoders, which I believe are
AsymmetricAutoencoderKLAutoencoderKLTemporalDecoderVQModel
should also inherit from AutoencoderMixin? It would make the autoencoder interface more consistent and I think it should be safe in the sense that e.g. AutoencoderMixin.enable_tiling should raise a NotImplementedError for these models and the associated tiling/slicing tests in AutoencoderTesterMixin should be properly skipped since these models don't define use_tiling/use_slicing attributes. (An exception is AsymmetricAutoencoderKL, which defines use_tiling/use_slicing but not enable_tiling/enable_slicing methods currently.)
dg845
left a comment
There was a problem hiding this comment.
LGTM :). I would suggest removing the use_slicing/use_tiling attributes on AsymmetricAutoencoderKL:
diffusers/src/diffusers/models/autoencoders/autoencoder_asym_kl.py
Lines 110 to 111 in cc65d0a
Since tiling/slicing isn't implemented for this model, I think it makes sense to skip e.g. AsymmetricAutoencoderKLTests.test_enable_disable_tiling (right now it passes since AsymmetricAutoencoderKL.enable_tiling is essentially a no-op).
I have removed them. But just a nit, I don't think the tests would actually, they should get skipped rather. |
|
Failing tests are unrelated. |

What does this PR do?
Asbtracts common methods like
enable_slicing(),disable_slicing(),enable_tiling(), anddisable_tiling()to a classAutoencoderMixin. Not all VAEs implement slicing and tiling and that has been addressed accordingly.As a consequence, we also reduce a bit of code bloat.