Skip to content
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

deps: update peft to install from pypi #265

Merged
merged 3 commits into from
Dec 13, 2023
Merged

Conversation

anhuong
Copy link
Contributor

@anhuong anhuong commented Nov 7, 2023

relates to: #263

Signed-off-by: Anh-Uong <anh.uong@ibm.com>
@anhuong
Copy link
Contributor Author

anhuong commented Nov 7, 2023

I tested this by running the tox unit tests and by running the Getting_Started jupyter notebook which ran successfully through the training. I haven't worked with caikit-nlp too much, is there another way that this should be tested? Also, it looks like that on the next release tag it will trigger the pypi push - does this need to be tested?

@gkumbhat
Copy link
Collaborator

gkumbhat commented Nov 7, 2023

@anhuong thanks for fixing the package here @anhuong . We would need to do an end to end training test with this, to make sure the training goes through smoothly for seq2seq and causal-lm models and also we are able to do inferencing on the tuned prompts. 🤔

pyproject.toml Outdated Show resolved Hide resolved
Co-authored-by: Gabe Goodhart <gabe.l.hart@gmail.com>
Signed-off-by: Anh Uong <anhuong4444@gmail.com>
@dtrifiro
Copy link
Contributor

dtrifiro commented Nov 8, 2023

How about just pinning peft==0.6.0 and letting dependabot submit PRs for updates?

Copy link
Collaborator

@gkumbhat gkumbhat left a comment

Choose a reason for hiding this comment

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

LGTM

@gkumbhat
Copy link
Collaborator

@anhuong actually lets go with @dtrifiro 's idea of pinning PEFT. That would be a more resilient way of handling for this case.

Copy link
Collaborator

@gkumbhat gkumbhat left a comment

Choose a reason for hiding this comment

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

Lets go with @dtrifiro's suggestion of pinning peft, that would be a bit more resilient to sudden or surprise breakage

Signed-off-by: Anh-Uong <anh.uong@ibm.com>
@evaline-ju evaline-ju linked an issue Dec 7, 2023 that may be closed by this pull request
3 tasks
@evaline-ju evaline-ju dismissed gkumbhat’s stale review December 12, 2023 20:33

going to unblock due to maintainer availability, since requested changes were addressed

@evaline-ju
Copy link
Collaborator

evaline-ju commented Dec 12, 2023

Verified training ability and inference after deployment (PT, MPT on causal lm [bloom] and seq2seq [flan-t5-xl])

Edit: update after revert of HF trainer in 0.4.0 - I verified that results were consistent on inference for the same models across version changes

Copy link
Collaborator

@evaline-ju evaline-ju left a comment

Choose a reason for hiding this comment

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

LGTM

@evaline-ju evaline-ju merged commit 61d10c0 into caikit:main Dec 13, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update peft dependency to 0.6.0 release
5 participants