Skip to content

Conversation

@a-r-r-o-w
Copy link
Contributor

What does this PR do?

  • Refactor some of the SD3 modeling implementation. Accessing configuration parameters in __init__ via the config attribute is not necessary or clean. Can use the parameters directly
  • Improve docs
  • Speedup tests

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@a-r-r-o-w a-r-r-o-w changed the title refactor refactor SD3, speedup tests Aug 13, 2024
@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.

@a-r-r-o-w
Copy link
Contributor Author

@DN6 LMK your thoughts here. Will update the expected test slices after approval

@a-r-r-o-w a-r-r-o-w requested a review from DN6 August 15, 2024 10:46
self.inner_dim = num_attention_heads * attention_head_dim

self.pos_embed = PatchEmbed(
height=self.config.sample_size,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think whether to use the config object inside models is more of a collective decision. We have it set up like this in a few places. Functionally, they are equivalent but we just need to commit to an approach. My personal preference is to use the variable directly as done here. cc: @yiyixuxu @sayakpaul

Copy link
Member

Choose a reason for hiding this comment

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

Yeah using a variable directly is what I have been following recently as well. During the forward() if the variable is accessible through self.config that takes priority, but if not (such as inner_dim) just assign then to self during __init__().

@a-r-r-o-w a-r-r-o-w marked this pull request as ready for review September 3, 2024 08:12
@a-r-r-o-w
Copy link
Contributor Author

Will update test slices which are needed due to making models smaller

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions bot added the stale Issues that haven't received updates label Sep 27, 2024
@a-r-r-o-w a-r-r-o-w removed the stale Issues that haven't received updates label Sep 27, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2024

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions bot added the stale Issues that haven't received updates label Nov 9, 2024
@sayakpaul
Copy link
Member

@a-r-r-o-w is this still relevant?

@a-r-r-o-w
Copy link
Contributor Author

@sayakpaul This is still relevant, yes. We were waiting for SD3.5 to be in when this went stale, so I will revive this soon as it's in now.

@github-actions github-actions bot removed the stale Issues that haven't received updates label Nov 10, 2024
@a-r-r-o-w a-r-r-o-w added the wip label Nov 18, 2024
@a-r-r-o-w a-r-r-o-w marked this pull request as draft January 1, 2025 21:55
@a-r-r-o-w a-r-r-o-w closed this Jan 1, 2025
@a-r-r-o-w a-r-r-o-w deleted the sd3/refactor-and-speedup-tests branch January 1, 2025 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants