-
Notifications
You must be signed in to change notification settings - Fork 445
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
Add option to request manual quit on tui #2489
base: main
Are you sure you want to change the base?
Conversation
I wasn't sure about the best way to implement this, particularly if adding it to the builder was the right approach. Let me know if you have any suggestions. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2489 +/- ##
==========================================
- Coverage 82.94% 82.56% -0.38%
==========================================
Files 811 827 +16
Lines 105059 106757 +1698
==========================================
+ Hits 87136 88143 +1007
- Misses 17923 18614 +691 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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.
I think that's a great start! But I have a couple of general comments.
First, I'm not sure adding with_manual_quit
is the best 🤔 especially since it doesn't apply to all renderers.
Instead, I think this could simply be added as an option for the TuiMetricsRenderer
and users could explicitly provide the renderer in with_renderer(...)
(at least for now).
Another thing I noticed is that it's not always clear when the training is done when keeping the TUI opened, especially if early stopping is used. Haven't had the time to look into it as much but it should be taken into consideration.
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.
Ah that's pretty good!
I think we could make one minimal change, after which the usage would look something like this:
let learner = LearnerBuilder::new(ARTIFACT_DIR)
// ...
.devices(vec![device.clone()])
.num_epochs(config.num_epochs)
.summary();
let tui = TuiMetricsRenderer::new(learner.interrupter(), None).persistent();
let learner = learner
.renderer(tui)
.build(Model::new(&device), config.optimizer.init(), 1e-4);
While it "breaks" the builder pattern, I think it's perfectly fine for now. We could eventually improve this but I don't think it's a critical issue atm.
Pull Request Template
Checklist
run-checks all
script has been executed.Related Issues/PRs
#2233
Changes
Add option to learner builder to wait for user to quit the renderer.
Testing
Manual test, TUI waits for q to close.