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
Add alpaca reverse augmentation possibility #2342
Conversation
❌ pre-commit failed. |
Eval dataset is still missing |
One thing that is not quite clear to me is if we should the reverse augmentation samples ADDITIONALLY to the train set |
Yes, I think forward + reverse could both be part of the training set. Ideally the same examples of the alpaca set should be used for evaluation of both forward/reverse (in order to avoid leaking of eval-data of the other dataset into the train set). |
For the smaller models it is necessary to specify |
Added the eval set and also the option to keep the unreversed data in the training set. |
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, thank you. IMO the now 'blood-empty' AlpacaDataset & CodeAlpacaDataset could be removed.
super().__init__() | ||
self.data = data | ||
if mode not in ["sft", "rl"]: |
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 be tuple instead of list
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.
done
class Alpaca(AlpacaBase): | ||
def __init__(self, mode: str = "sft", cache_dir: str = None) -> None: | ||
super().__init__(dataset_name="yahma/alpaca-cleaned", mode=mode, cache_dir=cache_dir) | ||
class AlpacaDataset(AlpacaBaseDataset): |
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.
If these classes don't add any functionality they could be removed (i.e. base class be used directly).
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.
done
manual_seed: int = 287631038922, | ||
reverse_augmentation: bool = False, | ||
keep_unreversed: bool = True, | ||
) -> tuple[AlpacaDataset, AlpacaDataset] | tuple[CodeAlpacaDataset, CodeAlpacaDataset]: |
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.
base class type annotation for factory function would be better imo
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.
done
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.
lgtm!
closes #2335
Alpaca with reverse augmentation can be run with:
I couldn't run this due to unscale gradient errors, so before this gets merged we should do a run to check whether this reversal really improves the loss.