-
Notifications
You must be signed in to change notification settings - Fork 63
Implement ELLA for SD1.5 #389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6a84669
to
aecb7d6
Compare
There was a problem hiding this 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
?
4ac9f44
to
ffb07d8
Compare
f4cea42
to
3d5d9ae
Compare
This bounty is stale because it has been opened for 7 days with no activity. |
There was a problem hiding this 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
I've run the tests, beside the fact that |
There was a problem hiding this 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?
This bounty is stale because it has been opened for 7 days with no activity. |
That's great!
OK, stay tuned.
@limiteinductive where is it used? ("[...] beside the fact that sentencepiece was missing from the dependencies, the 3 tests are green") |
Hi @ily-R, final nits before we merge:
|
c28191d
to
21b1096
Compare
21b1096
to
25c717f
Compare
I rebased and did the necessary lint/format and added the sentencepiece as a test dependency . The tests are green. |
There was a problem hiding this 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!
13f5bd1
to
1999efe
Compare
There was a problem hiding this 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)
@ily-R thanks for the quick turnaround. Some last suggestions (see above). |
1999efe
to
3dbdaf0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is my draft PR for the bounty ELLA for SD1.5. I implemented and tested the architecture. I have few steps left.