-
Notifications
You must be signed in to change notification settings - Fork 10
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
update benchmark_models cli #238
update benchmark_models cli #238
Conversation
* save file with --file only, to md, csv, or jsonl based on file extension
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 % comments
log10/cli/completions.py
Outdated
if path.suffix.lower() == ".csv": | ||
# save the dataframe to a csv file | ||
all_df.to_csv(path, index=False) | ||
rich.print(f"Dataframe saved to csv file: {path}") | ||
elif path.suffix.lower() == ".jsonl": | ||
all_df.to_json(path, orient="records", lines=True) | ||
rich.print(f"Dataframe saved to jsonl file: {path}") | ||
elif path.suffix.lower() == ".md": |
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.
we could extract the path.suffix.lower()
to be in a variable
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. reuse the name ext_name
at ln 312.
log10/cli/completions.py
Outdated
if path.suffix.lower() == ".csv": | ||
# save the dataframe to a csv file | ||
all_df.to_csv(path, index=False) | ||
rich.print(f"Dataframe saved to csv file: {path}") | ||
elif path.suffix.lower() == ".jsonl": | ||
all_df.to_json(path, orient="records", lines=True) | ||
rich.print(f"Dataframe saved to jsonl file: {path}") | ||
elif path.suffix.lower() == ".md": |
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.
can we also add the else
case to raise Error?
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.
we have checked the extension name at the beginning of the func (ln 310-316), so no need to check again here.
save file with --file only, to md, csv, or jsonl based on file extension