Skip to content

Conversation

ily-R
Copy link
Contributor

@ily-R ily-R commented Jul 16, 2024

This is my draft PR for the bounty ELLA for SD1.5. I implemented and tested the architecture. I have few steps left.

  • Implement ELLA
  • Test the adapter's inject/eject
  • implement the weight conversion script for these weights
  • test ELLA contribution to SD1.5 model using t5_xl text_embeddings from here

@ily-R ily-R force-pushed the feature/ella-SD1.5 branch 2 times, most recently from 6a84669 to aecb7d6 Compare July 23, 2024 19:05
Copy link
Contributor

@limiteinductive limiteinductive left a comment

Choose a reason for hiding this comment

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

Thanks for the great work.

I made my first round of essentially stylistic nits. I haven't made an e2e test, but verified that conversion is working.

Could you also apply lint using rye fmt and verify typing with rye run pyright?

@Laurent2916 Laurent2916 changed the title Feature/ella sd1.5 Implement ELLA for SD1.5 Jul 24, 2024
@ily-R ily-R force-pushed the feature/ella-SD1.5 branch from 4ac9f44 to ffb07d8 Compare July 25, 2024 20:03
@ily-R ily-R force-pushed the feature/ella-SD1.5 branch from f4cea42 to 3d5d9ae Compare July 29, 2024 15:37
Copy link
Contributor

github-actions bot commented Aug 6, 2024

This bounty is stale because it has been opened for 7 days with no activity.

@github-actions github-actions bot added the Stale label Aug 6, 2024
Copy link
Contributor

@Laurent2916 Laurent2916 left a comment

Choose a reason for hiding this comment

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

Hey @ily-R, this looks better!
Do you see anything else @limiteinductive ?
I'll run the tests tomorrow

@github-actions github-actions bot removed the Stale label Aug 7, 2024
@Laurent2916
Copy link
Contributor

I've run the tests, beside the fact that sentencepiece was missing from the dependencies, the 3 tests are green.

@ily-R ily-R requested a review from Laurent2916 August 7, 2024 13:56
Copy link
Contributor

@limiteinductive limiteinductive left a comment

Choose a reason for hiding this comment

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

I have nothing to add (except linting and rebase); @deltheil, could you have a final look? Also, what about the sentencepiece dependency?

Copy link
Contributor

This bounty is stale because it has been opened for 7 days with no activity.

@github-actions github-actions bot added the Stale label Aug 20, 2024
@deltheil
Copy link
Member

I have nothing to add (except linting and rebase);

That's great!

@deltheil, could you have a final look?

OK, stay tuned.

Also, what about the sentencepiece dependency?

@limiteinductive where is it used? ("[...] beside the fact that sentencepiece was missing from the dependencies, the 3 tests are green")

@Laurent2916
Copy link
Contributor

Hi @ily-R, final nits before we merge:

  • could you please add sentencepiece as a test dependency
  • because of a recent PR, there a small conflict in src/refiners/foundationals/latent_diffusion/stable_diffusion_1/__init__.py, could please resolve it

@Laurent2916 Laurent2916 added the run-ci Run CI label Aug 22, 2024
@github-actions github-actions bot removed the Stale label Aug 23, 2024
@ily-R ily-R force-pushed the feature/ella-SD1.5 branch 2 times, most recently from c28191d to 21b1096 Compare August 26, 2024 18:22
@ily-R ily-R requested a review from limiteinductive August 26, 2024 18:27
@ily-R ily-R force-pushed the feature/ella-SD1.5 branch from 21b1096 to 25c717f Compare August 26, 2024 19:42
@ily-R
Copy link
Contributor Author

ily-R commented Aug 26, 2024

I rebased and did the necessary lint/format and added the sentencepiece as a test dependency . The tests are green.
Let me know if there is something to fix

@deltheil deltheil added run-ci Run CI and removed run-ci Run CI labels Sep 2, 2024
Copy link
Member

@deltheil deltheil left a comment

Choose a reason for hiding this comment

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

@ily-R sorry for the delay - I sent a few final nit-s, do you mind having a look? Also, please rebase and squash into a single commit. Thanks!

@ily-R ily-R force-pushed the feature/ella-SD1.5 branch 2 times, most recently from 13f5bd1 to 1999efe Compare September 2, 2024 21:04
@ily-R ily-R requested a review from deltheil September 2, 2024 21:04
Copy link
Member

@deltheil deltheil left a comment

Choose a reason for hiding this comment

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

Please take a look (last round I guess)

@deltheil
Copy link
Member

deltheil commented Sep 3, 2024

@ily-R thanks for the quick turnaround. Some last suggestions (see above).

@ily-R ily-R force-pushed the feature/ella-SD1.5 branch from 1999efe to 3dbdaf0 Compare September 3, 2024 11:01
@ily-R ily-R requested a review from deltheil September 3, 2024 11:01
@deltheil deltheil added run-ci Run CI and removed run-ci Run CI labels Sep 4, 2024
Copy link
Contributor

@limiteinductive limiteinductive left a comment

Choose a reason for hiding this comment

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

LGTM

@deltheil deltheil merged commit 277b0fd into finegrain-ai:main Sep 4, 2024
2 checks passed
deltheil added a commit that referenced this pull request Sep 4, 2024
deltheil added a commit that referenced this pull request Sep 4, 2024
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.

4 participants