-
-
Notifications
You must be signed in to change notification settings - Fork 780
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
minor fixes 20231211 #943
minor fixes 20231211 #943
Conversation
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.
Could we also add nano
in base docker :)
@@ -191,6 +191,7 @@ def load_model( | |||
|
|||
# TODO refactor as a kwarg |
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.
there's this todo here.
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'm not even sure what this TODO is for atm
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.
maybe it's for the .from_pretrained
call where these are kwargs for that? should we add this to model_kwargs and remove those from all the from_pretrained calls?
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.
This is just above
load_in_8bit = cfg.load_in_8bit
load_in_4bit = cfg.load_in_4bit
@@ -535,7 +536,7 @@ def load_model( | |||
|
|||
model, lora_config = load_adapter(model, cfg, cfg.adapter) | |||
|
|||
if cfg.ddp and not load_in_8bit: | |||
if cfg.ddp and not load_in_8bit and not load_in_4bit: | |||
model.to(f"cuda:{cfg.local_rank}") |
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.
what's the side effect of this change? Does it mean, the models will all live on gpu0 for 4 bit now?
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.
there's an edge case that I had to have this set when doing DPO qlora for Mixstral. maybe it was a deepspeed thing and I should change this?
I found that you separated your apt installs into 2 separate docker file. Maybe, these should also be moved to base |
No description provided.