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

dvclive.log Refactor #164

Merged
merged 49 commits into from
Sep 17, 2021
Merged

dvclive.log Refactor #164

merged 49 commits into from
Sep 17, 2021

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Sep 15, 2021

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:

  • Added data module.
  • Extracted make_html to dvclive.dvc.
  • Extracted new method make_summary.

Added the following changes :

self.set_step(self.get_step() + 1)

This should fulfill the 3 points listed in #157 (comment), considering that checkpoints would be properly handled by DVC.


I'm starting to think that make_checkpoints, make_html, and make_summary could 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.

@daavoo daavoo requested a review from pared September 15, 2021 16:03
@daavoo daavoo marked this pull request as ready for review September 15, 2021 16:09
@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2021

Codecov Report

Merging #164 (8c17d63) into master (df9ed2b) will increase coverage by 0.41%.
The diff coverage is 96.42%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Ξ”
dvclive/utils.py 100.00% <ΓΈ> (ΓΈ)
dvclive/data/base.py 90.90% <90.90%> (ΓΈ)
dvclive/metrics.py 97.91% <96.87%> (+0.72%) ⬆️
dvclive/data/__init__.py 100.00% <100.00%> (ΓΈ)
dvclive/data/scalar.py 100.00% <100.00%> (ΓΈ)
dvclive/dvc.py 66.66% <100.00%> (+4.76%) ⬆️
dvclive/error.py 95.65% <100.00%> (+1.20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update df9ed2b...8c17d63. Read the comment docs.

Copy link
Contributor

@pared pared left a 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.

@daavoo daavoo merged commit 7143e3a into master Sep 17, 2021
@daavoo daavoo deleted the log-summary branch September 17, 2021 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dvclive.log: Remove the implicit next_step call . logger: log to summary first, then to logs
3 participants