Migrate train_on_inputs to sft-specific params #297
Conversation
src/together/cli/api/finetune.py
Outdated
There was a problem hiding this comment.
formatting only
connermanuel
force-pushed
the
cmanuel/eng-24978-move-train_on_inputs-to-the-parameters-of-sft-training
branch
2 times, most recently
from
8ef8769 to
1ce13d6
Compare
| ) | ||
| train_on_inputs = "auto" | ||
|
|
||
| if dpo_beta is not None and training_method != "dpo": |
There was a problem hiding this comment.
Other option might be just a warning. What do you think?
There was a problem hiding this comment.
good question! i don't have a strong opinion here, but i am leaning towards the error because non-None value means the user intentionally set it. if they intentionally set it, it might be because they assumed dpo would be active. so we should cancel and let them know that it isn't, rather than silently continuing with an sft job. wdyt @artek0chumak ?
There was a problem hiding this comment.
It's an imediate error, not something from FT API or from a job. I think it's fine to leave it us an error
There was a problem hiding this comment.
Don't forget version bump up: https://github.com/togethercomputer/together-python/blob/main/pyproject.toml#L15
connermanuel
force-pushed
the
cmanuel/eng-24978-move-train_on_inputs-to-the-parameters-of-sft-training
branch
from
01ab30b to
8331552
Compare
Co-authored-by: Artem Chumachenko <[email protected]>
connermanuel
force-pushed
the
cmanuel/eng-24978-move-train_on_inputs-to-the-parameters-of-sft-training
branch
from
8331552 to
77601a9
Compare
connermanuel
deleted the
cmanuel/eng-24978-move-train_on_inputs-to-the-parameters-of-sft-training
branch
this PR adjusts the behavior of train_on_inputs.
if train type is SFT, we include this parameter in the TrainingMethod and default it to auto.
if train type is DPO, we default to None and raise if parameter is supplied.