-
Notifications
You must be signed in to change notification settings - Fork 39
dvclive.log Refactor
#164
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
dvclive.log Refactor
#164
Conversation
for more information, see https://pre-commit.ci
β¦to log-summary
for more information, see https://pre-commit.ci
β¦to log-summary
Codecov Report
@@ Coverage Diff @@
## master #164 +/- ##
==========================================
+ Coverage 91.91% 92.32% +0.41%
==========================================
Files 15 17 +2
Lines 371 417 +46
==========================================
+ Hits 341 385 +44
- Misses 30 32 +2
Continue to review full report at Codecov.
|
pared
left a comment
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, I would consider creating some minor test (or updating test_nested_logging) with a check that verifies that first log updates summary.
β I have followed the Contributing to DVCLive guide.
π If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Included some refactor (sorry for the number of changes π) which wasn't strictly necessary to address the changes but I think would help in upcoming changes:
datamodule.make_htmltodvclive.dvc.make_summary.Added the following changes :
summaryis now generated indvclive.log. Closes logger: log to summary first, then to logsΒ #145dvclive.lograises a newAlreadyLoggedErrorif called with the same metric and step . Closesdvclive.log: Remove the implicitnext_stepcall .Β #162make_htmlandmake_checkpointare now called indvclive.set_step.dvclive.next_stepis now an alias forThis should fulfill the 3 points listed in #157 (comment), considering that
checkpointswould be properly handled by DVC.I'm starting to think that
make_checkpoints,make_html, andmake_summarycould be extracted to explicit public methods. Even though it might increase the complexity of the API, it should drive users to one of:a) Use our integrations.
b) This is our canonical workflow but feel free to alter it, there are no side effects.