Correct an apparent logger output directory bug #1232
Conversation
yutsai84
changed the title
|
Thanks for the contribution. I don't have a strong opinion on the hardcoding of |
Co-authored-by: Sebastian Raschka <mail@sebastianraschka.com>
There was a problem hiding this comment.
Approving in case there is a strong desire for this change, but personally I vote against it (see comment below). Note that unless we expose every little configuration option in every logger it will always be opinionated and "hardcoded". But I think in the current state of LitGPT, that's also ok.
| @@ -451,9 +451,9 @@ def choose_logger( | |||
| **kwargs: Any, | |||
| ): | |||
| if logger_name == "csv": | |||
| return CSVLogger(root_dir=(out_dir / "logs"), name="csv", flush_logs_every_n_steps=log_interval, **kwargs) | |||
| return CSVLogger(root_dir=(out_dir / "logs"), name=name, flush_logs_every_n_steps=log_interval, **kwargs) | |||
There was a problem hiding this comment.
I'd like to point out that "name" that is passed in is not conceptually the same as "name" argument of the logger, it's used differently to make a simplified directory structure. And so the choice here was deliberate (by me). With the change in this PR, there will now be essentially twice the name in the directory structure, which is what I wanted to avoid.
The
namearg is hardcoded in the main branch. This pr tries to correct it by using the value parsed by thenameargument.