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

studio: Don't skip data event if _inside_dvc_exp. #471

Closed
wants to merge 2 commits into from

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Feb 27, 2023

Workaround #397 for live experiments with dvc exp run.

Closes #473

Allows to send data updates when running inside dvc exp run and DVC is not available as a Python library.

Workaround #397 for live experiments with `dvc exp run`.

Allows to send live experiment updates when running inside `dvc exp run` and DVC is not available as a Python library.
@daavoo daavoo self-assigned this Feb 27, 2023
@daavoo daavoo added A: studio Area: Studio integration enhancement labels Feb 27, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2023

Codecov Report

Base: 95.71% // Head: 95.96% // Increases project coverage by +0.24% 🎉

Coverage data is based on head (b0a2d44) compared to base (3308c68).
Patch coverage: 84.48% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #471      +/-   ##
==========================================
+ Coverage   95.71%   95.96%   +0.24%     
==========================================
  Files          37       37              
  Lines        2336     2355      +19     
  Branches      196      196              
==========================================
+ Hits         2236     2260      +24     
+ Misses         61       57       -4     
+ Partials       39       38       -1     
Impacted Files Coverage Δ
src/dvclive/live.py 92.96% <72.41%> (+1.81%) ⬆️
src/dvclive/dvc.py 90.56% <87.50%> (+0.27%) ⬆️
tests/test_dvc.py 100.00% <100.00%> (ø)
tests/test_studio.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@dberenbaum
Copy link
Contributor

Does it work for repro (which is still much more common in CML)?

DVC is not available as a Python library

Can we better clarify in the code or tests that this is what it's for? It took me a bit to understand how self._dvc_repo was being used here, and it seems poorly named for this usage. Maybe we could have distinct methods/attributes to determine whether there is a DVC repo and whether DVC is installed as a lib?

@daavoo
Copy link
Contributor Author

daavoo commented Feb 27, 2023

Does it work for repro (which is still much more common in CML)?

No

Maybe we could have distinct methods/attributes to determine whether there is a DVC repo and whether DVC is installed as a lib?

Yes, we should do that. Addressing it should also improve the warnings as the current warning is misleading when DVC is not available as a lib.

@daavoo
Copy link
Contributor Author

daavoo commented Feb 27, 2023

Yes, we should do that. Addressing it should also improve the warnings as the current warning is misleading when DVC is not available as a lib.

Will just do it in this P.R.

@daavoo
Copy link
Contributor Author

daavoo commented Feb 27, 2023

Does it work for repro (which is still much more common in CML)?

To add on this, I just feel like we should not support live experiments with dvc repro

@dberenbaum
Copy link
Contributor

To add on this, I just feel like we should not support live experiments with dvc repro

Let's discuss in #474 😄

@dberenbaum
Copy link
Contributor

@daavoo Is it ready for another review?

from ruamel.yaml.representer import RepresenterError

from . import env
from .dvc import (
dvc_installed,
Copy link
Member

Choose a reason for hiding this comment

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

minor: name is a bit misleading ... dvc_lib_available?

@daavoo daavoo force-pushed the live-metrics-inside-dvc-exp branch 2 times, most recently from 9002f3a to 3ed7b2f Compare March 1, 2023 15:54
@daavoo
Copy link
Contributor Author

daavoo commented Mar 1, 2023

@daavoo Is it ready for another review?

@dberenbaum yes

Comment on lines -32 to -38
try:
from dvc_studio_client.env import STUDIO_TOKEN
from dvc_studio_client.post_live_metrics import post_live_metrics
except ImportError:
post_live_metrics = None
STUDIO_TOKEN = None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated but dvc_studio_client has been in install_requires of dvclive for a while

@dberenbaum
Copy link
Contributor

A couple questions:

  1. This looks like it prioritizes warning about installing DVC lib over having a DVC repo. Maybe it's better to always check both and provide both warnings so people don't solve one error only to run into the next?
  2. It doesn't need to block this PR, but it feels like with the increasing integration with DVC, we need to come to some resolution on remove dvc lib requirement #397 and consider in what scenarios DVCLive makes sense without DVC or a DVC repo. It may be better to require these generally rather than check for them for enabling each individual feature.

@daavoo daavoo force-pushed the live-metrics-inside-dvc-exp branch from 3ed7b2f to 1f6b9bd Compare March 2, 2023 12:02
@daavoo
Copy link
Contributor Author

daavoo commented Mar 2, 2023

1. This looks like it prioritizes warning about installing DVC lib over having a DVC repo. Maybe it's better to always check both and provide both warnings so people don't solve one error only to run into the next?

Done.

2. It doesn't need to block this PR, but it feels like with the increasing integration with DVC, we need to come to some resolution on [remove dvc lib requirement #397](https://github.com/iterative/dvclive/issues/397) and consider in what scenarios DVCLive makes sense without DVC or a DVC repo. It may be better to require these generally rather than check for them for enabling each individual feature.

Let's discuss in the issue

@daavoo
Copy link
Contributor Author

daavoo commented Mar 2, 2023

Closing in favor of #481

@daavoo daavoo closed this Mar 2, 2023
@daavoo daavoo deleted the live-metrics-inside-dvc-exp branch March 6, 2023 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: studio Area: Studio integration enhancement
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Decouple DVC repo existence from DVC available as a lib
4 participants