-
Notifications
You must be signed in to change notification settings - Fork 968
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
OptimizerArgs #1409
OptimizerArgs #1409
Conversation
Your jsonargparse example has been super helpful for understanding things a bit more @carmocca . Many thanks for this! But maybe it's because it's Fri evening but my brain is just not working today. I've just been banging my head against how I would get this into the finetuning method's Adding the
But we can't add Also, how would we get the
if we don't pass them on from the
in the finetuning script, but then it would erase all the previous arguments. |
As far as integrating into the scripts, I would: Create an optimizer argument in litgpt/litgpt/finetune/lora.py Line 40 in 36c6a77
To avoid the duplicate registration, you need to skip it when the function arguments are added Line 121 in 36c6a77
And call litgpt/litgpt/finetune/lora.py Lines 185 to 187 in 36c6a77
This should be enough to unblock you. The not-so-nice thing is that the CLI args structure leaks into the actual script, meaning that users who don't go through the CLI will have to create this dictionary manually. |
Awesome, thanks so much, this was great help! Figured it out now and got it to work. Many thanks, again learned something new! |
I now got it to work as follows: litgpt finetune full \
--checkpoint_dir checkpoints/EleutherAI/pythia-160m
# Specify optimizer and optimizer args:
litgpt finetune full \
--checkpoint_dir checkpoints/EleutherAI/pythia-160m \
--optimizer torch.optim.SGD \
--optimizer.init_args.lr 1000 But I feel like the way I am passing the optimizer kwargs seems a bit hacky. Is this there a built-in/better way to handle it @carmocca ? The thing is that when I pass an kwargs = {
'optimizer.class_path': 'torch.optim.SGD',
'optimizer.init_args.dampening': 0.0,
'optimizer.init_args.differentiable': False,
'optimizer.init_args.foreach': None,
'optimizer.init_args.lr': 0.001,
'optimizer.init_args.maximize': False,
'optimizer.init_args.momentum': 0.0,
'optimizer.init_args.nesterov': False,
'optimizer.init_args.weight_decay': 0.0
} That's why I added the parsing into class_path and init_args: optimizer_class_path = None
optimizer_init_args = {}
for key, value in list(kwargs.items()):
if key.startswith("optimizer"):
if "class_path" in key:
optimizer_class_path = value
elif "init_args" in key:
init_arg_key = key.split(".")[-1]
optimizer_init_args[init_arg_key] = value
del kwargs[key] Everything seems to work, but I wonder if there isn't a better way to do it? |
@rasbt I pushed a commit with what I would suggest. The Also fyi, you don't need to specify the |
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.
Wow awesome, thanks so much for simplifying this. Jsonargparse is still a bit of a black box for me.
The only caveat now is that the class path still needs to be specified. I.e., only specifying the learning rate doesn't work
And the optimizer always needs to be specified explicitely litgpt finetune full --optimizer AdamW --optimizer.lr 200 --checkpoint_dir checkpoints/EleutherAI/pythia-160m Do you know if that's a jsonargparse thing, @carmocca ? Because we already set a default value in the |
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.
Added AdamW as the CLI default so that you don't have to specify it
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
I hope this is ready now @carmocca |
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.
Very nice job!
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
The azure failure does look real: > fit(fabric, devices, state, train_dataloader, val_dataloader, out_dir, tokenizer_dir, train, eval, optimizer)
E TypeError: fit() takes 9 positional arguments but 10 were given
/__w/6/s/extensions/thunder/pretrain.py:229: TypeError
----------------------------- Captured stderr call -----------------------------
Missing logger folder: /tmp/pytest-of-root/pytest-0/test_pretrain0/out/logs/tensorboard
Seed set to 42
=========================== short test summary info ============================
FAILED tests/test_thunder_pretrain.py::test_pretrain - TypeError: fit() takes 9 positional arguments but 10 were given |
It does. Let me investigate ... |
Should be fixed for good now @carmocca . I can switch the link to the original tinystories now that you have seen the green checks haha 😆 |
This PR unbundles the OptimizerArgs approach from GaLore in #1192.
Todos
OptimizerArgs for full finetuning
OptimizerArgs for LoRA
OptimizerArgs for Adapter
OptimizerArgs for Adapter v2
OptimizerArgs for Pretraining
ensure that both
--optimizer torch.optim.AdamW
and--optimizer AdamW
worksAdd tests