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

add cache_images to lightning #614

Merged
merged 4 commits into from
Jul 4, 2023
Merged

add cache_images to lightning #614

merged 4 commits into from
Jul 4, 2023

Conversation

dberenbaum
Copy link
Collaborator

Adds dvclive.lightning.DVCLiveLogger(cache_images=True)

@dberenbaum dberenbaum requested a review from daavoo July 3, 2023 20:19
@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.04 ⚠️

Comparison is base (a0de508) 89.72% compared to head (bdb9c12) 89.69%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #614      +/-   ##
==========================================
- Coverage   89.72%   89.69%   -0.04%     
==========================================
  Files          43       43              
  Lines        2987     2988       +1     
  Branches      248      248              
==========================================
  Hits         2680     2680              
- Misses        268      269       +1     
  Partials       39       39              
Impacted Files Coverage Δ
src/dvclive/lightning.py 0.00% <ø> (ø)
tests/test_frameworks/test_lightning.py 6.89% <0.00%> (-0.05%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -44,6 +44,7 @@ def __init__(
report: Optional[str] = "auto",
save_dvc_exp: bool = False,
dvcyaml: bool = True,
cache_images: bool = False,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to also add it to the _live_init dict below.

@dberenbaum
Copy link
Collaborator Author

@daavoo Any ideas for how to test when adding a new Live() option that it also exists in the lightning logger?

@daavoo
Copy link
Contributor

daavoo commented Jul 4, 2023

@daavoo Any ideas for how to test when adding a new Live() option that it also exists in the lightning logger?

I guess we need to remember to update this test with the new args:

def test_lightning_kwargs(tmp_dir):
model = LitXOR()
# Handle kwargs passed to Live.
dvclive_logger = DVCLiveLogger(dir="dir", report="md", dvcyaml=False)
trainer = Trainer(
logger=dvclive_logger,
max_epochs=2,
enable_checkpointing=False,
log_every_n_steps=1,
)
trainer.fit(model)
assert os.path.exists("dir")
assert os.path.exists("dir/report.md")
assert not os.path.exists("dir/dvc.yaml")

Seems like the reasonable thing to do since we don't change or have that many args and it is the only framework where the args are not directly passed like in other frameworks as Live(**kwargs)

@dberenbaum
Copy link
Collaborator Author

Seems like the reasonable thing to do since we don't change or have that many args and it is the only framework where the args are not directly passed like in other frameworks as Live(**kwargs)

Added a test, but I think not changing it often is why I forget to do it. Anyways, I don't have any better ideas for now. Just a thought.

@daavoo daavoo merged commit d261760 into main Jul 4, 2023
@daavoo daavoo deleted the cache-images-lightning branch July 4, 2023 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants