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

Change llm judge to rm/generate interface #293

Merged
merged 11 commits into from
Dec 14, 2024
Merged

Conversation

Kipok
Copy link
Collaborator

@Kipok Kipok commented Dec 12, 2024

No description provided.

Signed-off-by: Igor Gitman <[email protected]>
Copy link
Collaborator

@gwarmstrong gwarmstrong left a comment

Choose a reason for hiding this comment

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

Largely LGTM. One minor comment.

output_dir: str | None = None
# Used to identify the input file if `input_dir` is provided. If `random_seed` is not provided,
# the input will be assumed to be from 'greedy' generation
random_seed: str | None = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why str? Int's should be nullable, but you might have to use null from cmdline instead of None. Perplexity example here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just copied whatever was in the reward model script without checking. Let me update both

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

on a second thought, let me keep it as is for now - I think eventually we should move this parameter out of here to the pipeline part

@Kipok Kipok merged commit 197d090 into main Dec 14, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants