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

Fix QLoRA weights and bias initialisation #833

Merged
merged 10 commits into from
Jan 21, 2025
Merged

Conversation

caroteu
Copy link
Contributor

@caroteu caroteu commented Jan 15, 2025

@anwai98 This includes the QLoRA initialisation changes and this also works for inference now. The changes that made the inference work are a bit hacky though, so we should think about a better/safer way to do this.

Copy link
Contributor

@anwai98 anwai98 left a comment

Choose a reason for hiding this comment

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

Hi @caroteu,

I left some comments below. In general, I feel using strict=False is a bit tricky (and error-prone) to set it for loading model checkpoints. We should discuss the issues and find an elegant way to take care of this!

micro_sam/instance_segmentation.py Outdated Show resolved Hide resolved
micro_sam/models/peft_sam.py Show resolved Hide resolved
micro_sam/util.py Outdated Show resolved Hide resolved
micro_sam/util.py Outdated Show resolved Hide resolved
@caroteu
Copy link
Contributor Author

caroteu commented Jan 15, 2025

@anwai98
The way we handle the QLoRA state dict is now updated to make things safer as discussed

Copy link
Contributor

@anwai98 anwai98 left a comment

Choose a reason for hiding this comment

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

Hi @caroteu,

I merged the previous PR here. Can we briefly touch down on the comments below?

micro_sam/instance_segmentation.py Outdated Show resolved Hide resolved
micro_sam/util.py Outdated Show resolved Hide resolved
micro_sam/util.py Outdated Show resolved Hide resolved
micro_sam/models/peft_sam.py Show resolved Hide resolved
micro_sam/util.py Outdated Show resolved Hide resolved
micro_sam/util.py Outdated Show resolved Hide resolved
caroteu and others added 2 commits January 21, 2025 17:00
Does the inference for QLoRA-finetuned models properly!
@anwai98
Copy link
Contributor

anwai98 commented Jan 21, 2025

I added docstrings to our new export function (and thanks to @caroteu for making an elegant solution out of this).

This is GTG from my side.

PS. @caroteu @constantinpape It would be nice to go over this and see if everything makes sense now!

Copy link
Contributor

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

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

This looks good to me. Feel free to merge.

@anwai98 anwai98 merged commit 552dc55 into dev Jan 21, 2025
3 checks passed
@anwai98 anwai98 deleted the fix-qlora-initialisation branch January 21, 2025 17:05
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.

3 participants