Skip to content

Conversation

@sayakpaul
Copy link
Member

What does this PR do?

Asbtracts common methods like enable_slicing(), disable_slicing(), enable_tiling(), and disable_tiling() to a class AutoencoderMixin. Not all VAEs implement slicing and tiling and that has been addressed accordingly.

As a consequence, we also reduce a bit of code bloat.

@HuggingFaceDocBuilderDev

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.

@sayakpaul sayakpaul requested a review from dg845 October 19, 2025 19:05
Copy link
Collaborator

@dg845 dg845 left a comment

Choose a reason for hiding this comment

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

LGTM! Do you think the remaining autoencoders, which I believe are

  • AsymmetricAutoencoderKL
  • AutoencoderKLTemporalDecoder
  • VQModel

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.)

@sayakpaul sayakpaul requested a review from dg845 October 21, 2025 06:19
Copy link
Collaborator

@dg845 dg845 left a comment

Choose a reason for hiding this comment

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

LGTM :). I would suggest removing the use_slicing/use_tiling attributes on AsymmetricAutoencoderKL:

self.use_slicing = False
self.use_tiling = False

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).

@sayakpaul
Copy link
Member Author

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.

@sayakpaul
Copy link
Member Author

Failing tests are unrelated.

@sayakpaul sayakpaul merged commit a5a0ccf into main Oct 22, 2025
13 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants