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

[feat] Add Accelerate SFT Trainer #280

Merged
merged 11 commits into from
Feb 8, 2023
Merged

[feat] Add Accelerate SFT Trainer #280

merged 11 commits into from
Feb 8, 2023

Conversation

@jon-tow jon-tow mentioned this pull request Feb 6, 2023
@maxreciprocate maxreciprocate marked this pull request as ready for review February 7, 2023 14:14
@cat-state
Copy link
Collaborator

Thanks @reciprocated , this mostly LGTM, could you update the other examples to also match the samples, rewards args instead of dataset?

@cat-state cat-state self-requested a review February 7, 2023 23:49
@maxreciprocate
Copy link
Collaborator Author

@cat-state, I've updated those, but perhaps I was overzealous with making TRLConfig a positional and required argument, since the only example that doesn't work is simulacra.py and by extension also simulacra's colab which git cloned main, since it didn't provide a config of its own. Should I maybe revert those changes? Not sure how much have you progressed with replacing the current configuration, and how much the above still matters

from typing import Callable, Dict, Iterable, List, Optional, Tuple

from trlx.data.configs import TRLConfig
from trlx.utils import set_seed
from trlx.utils.loading import get_orchestrator, get_pipeline, get_trainer


def train(
def train( # noqa: C901
config: Optional[TRLConfig] = None,
model_path: Optional[str] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe model path should still be the first arg? It's also whats still in the readme.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, reverted that bit

@cat-state
Copy link
Collaborator

cat-state commented Feb 8, 2023

For the future config changes, we'll still need to keep the ModelConfig part around so keeping model_path makes sense for now. We can update the simulacra colab to give a model_path which should make this work?
I don't think you need to revert the changes made to the examples/config usage.

Copy link
Collaborator

@cat-state cat-state left a comment

Choose a reason for hiding this comment

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

okay! This LGTM now

@maxreciprocate maxreciprocate merged commit 888ae11 into main Feb 8, 2023
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.

None yet

2 participants