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

Improve fine-tuning CLI messages with rich #191

Merged
merged 3 commits into from
Sep 24, 2024
Merged

Improve fine-tuning CLI messages with rich #191

merged 3 commits into from
Sep 24, 2024

Conversation

mryab
Copy link
Member

@mryab mryab commented Sep 24, 2024

When users submit a command to fine-tune a model at the moment, the report message prints out the entire response to the fine-tuning request, which contains a lot of technical information and is difficult to understand. This PR replaces that with logging of FinetuneRequest instead before the job is submitted and prints the identifier of the job along with its time when the job is accepted by the API.

The difference is easiest to see in screenshots.

Before:
image
After:
image

Fixes ENG-7292

from textwrap import wrap

import click
from click.core import ParameterSource # type: ignore[attr-defined]
from rich import print as rich_print

Choose a reason for hiding this comment

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

in one place you import as rich_print, in other as just print
let's import always as "print"? I believe it's 1to1 replacement for print so should be fine

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not a big fan of replacing builtins, as it makes understanding the code more difficult (you don't know which function is called). Rich suggests in their docs to use rprint, so I will switch to that

@orangetin orangetin merged commit e4cbf4c into main Sep 24, 2024
13 checks passed
@orangetin orangetin deleted the print_with_rich branch September 24, 2024 15:34
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.

4 participants