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 "side effects" #156

Closed
2 tasks done
daavoo opened this issue Sep 9, 2021 · 6 comments · Fixed by #167
Closed
2 tasks done

dvclive.log "side effects" #156

daavoo opened this issue Sep 9, 2021 · 6 comments · Fixed by #167
Labels
discussion requires active participation to reach a conclusion p1-important Include in the next sprint refactoring

Comments

@daavoo
Copy link
Contributor

daavoo commented Sep 9, 2021

I would like to discuss the current behavior of dvclive.log which, in certain situations, replaces dvclive.init and calls dvclive.next_step.

It is not clear for me the tradeoff between benefits and potential problems of these "side effects" and would like to know about potential benefits I might be missing.

Assuming that the primary usage of dvclive would be through our provided integrations, end users won't really care about how we handle the workflow inside those integrations. For those using the API, the current behavior might be confusing (#289).

I think that we should enforce a canonical workflow (init -> log -> next_step) in both docs and integrations by:

@daavoo daavoo added discussion requires active participation to reach a conclusion refactoring labels Sep 9, 2021
@daavoo
Copy link
Contributor Author

daavoo commented Sep 9, 2021

cc @dberenbaum @pared

@dberenbaum
Copy link
Collaborator

@pared will know better about the reasons for the current implementation, but I assume it's convenience of being able to skip unnecessary dvclive.init() and dvclive.next_step() calls, as well as maybe being flexible for users who might forget to call those. However, I don't feel strongly about whether this added convenience is worth the tradeoffs of unclear implicit behavior.

Not sure if this belongs in this issue, but what is the advantage of dvclive.init() over logger = dvclive.MetricsLogger() as the canonical workflow? Explicitly calling MetricsLogger seems more transparent, removes an extra layer of abstraction, and makes it easier to manage multiple loggers.

@daavoo
Copy link
Contributor Author

daavoo commented Sep 10, 2021

Not sure if this belongs in this issue, but what is the advantage of dvclive.init() over logger = dvclive.MetricsLogger() as the canonical workflow? Explicitly calling MetricsLogger seems more transparent, removes an extra layer of abstraction, and makes it easier to manage multiple loggers.

My intention was to later treat that exact point. I think that, in addition to what you pointed, it could also simplify some things like get_step / set_step as those would be unnecessary and become a plain step Python @property wich people could manipulate.

@daavoo daavoo mentioned this issue Sep 10, 2021
2 tasks
@dberenbaum
Copy link
Collaborator

Related: #36

@pared
Copy link
Contributor

pared commented Sep 14, 2021

being able to skip unnecessary dvclive.init() and dvclive.next_step() calls, as well as maybe being flexible for users who might forget to call those

It clearly seems that out-of-the-box functionality makes the learning curve steeper. If that makes using dvclive confusing we should probably stop infering the users intentions and just require proper workflow.

Not sure if this belongs in this issue, but what is the advantage of dvclive.init() over logger = dvclive.MetricsLogger() as the canonical workflow?

User convenience, same as above.
If we begin to teach by providing MetricLogger we don't have to handle iterative/dvc.org#36 at all. In that case, we need to remember that we would need to move from_env as @daavoo mentioned.

@dberenbaum
Copy link
Collaborator

dberenbaum commented Sep 14, 2021

Can we emphasize MetricsLogger -> log -> next_step but still keep for i in range(epochs): dvclive.log() as a simplified workflow for ease of use in simple scenarios? We could get rid of dvclive.init() and dvclive.next_step() but keep dvclive.log() since this is less for users to remember in simple scenarios. Or do you think that could still cause confusion?

Edit: I guess this would create inconsistency between an implicit next_step in dvclive.log() and an explicit one in MetricsLogger, so it doesn't seem to make sense to keep dvclive.log().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion requires active participation to reach a conclusion p1-important Include in the next sprint refactoring
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants