search

iterative / dvclive

📈 Log and track ML metrics, parameters, models with Git and/or DVC
https://dvc.org/doc/dvclive
Apache License 2.0
161 stars 35 forks source link

Adds lightning fabric integration #749

Closed dberenbaum closed 8 months ago

dberenbaum commented 9 months ago

Closes #742. Fabric is Pytorch Lightning's attempt to abstract out the lightweight parts of their codebase so users can benefit from those without having to entirely adapt Lightning.

The existing Lightning logger was updated to inherit from this one. To make everything work with the existing Lightning logger, I added a sync method that decouples next_step from saving all the info to Studio, dvc.yaml, etc. I made it public, but no strong opinion on whether it should be public or documented.

See the Tensorboard logger in lightning for an example of another logger:

codecov-commenter commented 9 months ago

Codecov Report

Attention: 130 lines in your changes are missing coverage. Please review.

Comparison is base (8406920) 88.79% compared to head (2f81b3f) 86.81%.

Files Patch % Lines
src/dvclive/fabric.py 6.75% 69 Missing :warning:
tests/frameworks/test_fabric.py 25.00% 27 Missing :warning:
tests/frameworks/test_lightning.py 0.00% 17 Missing :warning:
src/dvclive/lightning.py 0.00% 16 Missing :warning:
src/dvclive/utils.py 50.00% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #749 +/- ## ========================================== - Coverage 88.79% 86.81% -1.99% ========================================== Files 53 55 +2 Lines 3357 3450 +93 Branches 294 304 +10 ========================================== + Hits 2981 2995 +14 - Misses 337 416 +79 Partials 39 39 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

shcheklein commented 9 months ago

Great work! Thanks, @dberenbaum for pushing forward integrations. Some comments as I'm reviewing / running it:

shcheklein commented 9 months ago

@dberenbaum looks good to me overall (I didn't review really deep the Fabric existing loggers to actually deeply review the implementation, but overall it looks fine). I've enabled mypy back + left a comment about VS Code to review, probably also Studio needs to be reviewed / tried out.

dberenbaum commented 9 months ago

Should mypy be included in https://github.com/iterative/dvclive/blob/main/.pre-commit-config.yaml?

shcheklein commented 9 months ago

I would ask @skshetry and the team on what the current state of the art for this :)

dberenbaum commented 9 months ago

@skshetry How should mypy checks be handled?

dberenbaum commented 8 months ago

ping @skshetry

dberenbaum commented 8 months ago

I'm able to see it in VS Code and Studio:

https://github.com/iterative/dvclive/assets/2308172/ce55849b-6825-4704-b934-ab85234f464f

https://github.com/iterative/dvclive/assets/2308172/dec278b2-f290-42de-b169-e49c22979389

skshetry commented 8 months ago

I would ask @skshetry and the team on what the current state of the art for this :)

We use nox for environment management on py-template based projects. So, you can pip install nox and then nox -s lint will take care of everything related to linting.

Since dvclive seems to have removed nox, you can try adding mypy to pre-commit but up to you. Look into dvc's pre-commit config.

dberenbaum commented 8 months ago

Depends on #757 now. The remaining test failure is unrelated.