add success messages for generate train convert evaluate test cmds#2741
add success messages for generate train convert evaluate test cmds#2741mergify[bot] merged 1 commit intoinstructlab:maininstructlab/instructlab:mainfrom reidliu41:add-success-msg-for-cmdsreidliu41/instructlab:add-success-msg-for-cmdsCopy head branch name to clipboard
Conversation
b62f34f to
d3f03d2
Compare
nathan-weinberg
left a comment
There was a problem hiding this comment.
Honestly, I like the emojis - it's a go from Nathan
src/instructlab/model/evaluate.py
Outdated
| turn2_score = round(turn2_score, 2) | ||
| print(turn2_score) | ||
| display_error_rate(error_rate) | ||
| click.echo("\n✅ Model evaluate with MTBENCH completed successfully!") |
There was a problem hiding this comment.
MTBENCH here and MtBenchBranch below are inconsistent.
I am not really sure this is helpful for the eval cases. Given the results are displayed at the end, it seems pretty obvious already.
nit: Note that it's also possible you have a very high error rate and it's not really a "success"
There was a problem hiding this comment.
Sorry, what do you mean of inconsistent ?
Yeah, may remove successfully , just mention finish.
There was a problem hiding this comment.
I was referring to the case differences between MTBENCH and MtBenchBranch
There was a problem hiding this comment.
Is it ok for MTBench and MTBenchBranch (check it from README.md)?
src/instructlab/cli/model/train.py
Outdated
| eval_gpus=ctx.obj.config.evaluate.gpus, | ||
| training_journal=training_journal, | ||
| ) | ||
| click.echo("✅ Model train with accelerated completed successfully!") |
There was a problem hiding this comment.
These are all tricky since other messages are being printed throughout the code. For example with case I think you might already get a:
Training finished! Best final checkpoint: /tmp/tmp.01L0NjBZlG/.local/share/instructlab/skills-only/phase2/checkpoints/hf_format/samples_616 with score: 8.368421052631579
Also nit: This message might be better worded as:
Accelerated model training completed successfully!
(Similar for the other train messages below)
There was a problem hiding this comment.
yeah, just checked, seems finished only for accelerated train, there is no msg for simple and full
src/instructlab/cli/data/generate.py
Outdated
| system_prompt, | ||
| legacy_pretraining_format, | ||
| ) | ||
| click.echo("✅ Data generate completed successfully!") |
There was a problem hiding this comment.
Any concerns about character compatibility with the ✅ across terminals or other supported environments?
There was a problem hiding this comment.
re-considering whether can add a simple check first.
|
I really like the ✅ but I also worry about how this will render especially in log files. could we do some ascii drawing or something like we have when training starts?
ᕙ(•̀‸•́‶)ᕗ |
i think is good to have a try |
d3f03d2 to
d782769
Compare
Signed-off-by: reid_liu <guliu@redhat.com>
0810710 to
9566507
Compare
|
FYI this didn't fail in the pre-merge testing, but will break all the large/xlarge CI jobs until we change Line 292 in 27cf6e4 |
For #2741 (comment) **Checklist:** - [ ] **Commit Message Formatting**: Commit titles and messages follow guidelines in the [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/#summary). - [ ] [Changelog](https://github.com/instructlab/instructlab/blob/main/CHANGELOG.md) updated with breaking and/or notable changes for the next minor release. - [ ] Documentation has been updated, if necessary. - [ ] Unit tests have been added, if necessary. - [ ] Functional tests have been added, if necessary. - [ ] E2E Workflow tests have been added, if necessary. Approved-by: cdoern Approved-by: bbrowning Approved-by: nathan-weinberg
for 2740
Checklist:
conventional commits.