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.init: Future of arguments #128

Closed
daavoo opened this issue Jul 29, 2021 · 9 comments · Fixed by #142
Closed

dvclive.init: Future of arguments #128

daavoo opened this issue Jul 29, 2021 · 9 comments · Fixed by #142
Labels
discussion requires active participation to reach a conclusion enhancement maintenance

Comments

@daavoo
Copy link
Contributor

daavoo commented Jul 29, 2021

Discussion regarding dvclive.init and it's number of arguments have emerged across multiple issues (i.e. #69, #75, #121, #124), specially those regarding the addition of new features, so I opened this one to concentrate all discussions.

From an user perspective I prefer having as many explicit arguments as needed, as long as they are useful and well documented, rather than "hiding" the arguments under **kwargs** or even a config file. Maybe I'm biased because I feel that using APIs with too many arguments (~8+) is not a real problem if all of them are actually useful, the library uses things like proper naming and types and you are using a modern IDE.

I would need to think about it and consider other perspectives to actually have a strong preference,.


Regardless of the options for extending arguments, I think it could be also worth to take a step back and consider the general interface to check if some of the arugments already existing could be removed/revisited in order to "make room" for new ones:

  • html

Given that this argument is only used when DVCLive is used alongside DVC. I think it could be removed from dvclive.init and just let it be configured by env.DVCLIVE_HTML.

  • step / resume

I'm not really sure what would be the indented use case for this argument outside it's interaction with resume. Unless I'm missing some other use case, resume and step could be merged into a single argument resume_from: Optional[int] = None?

  • summary

How much would this actually be used to disable the summary? Would someone complain if we remove the argument and just always create it?

If we come to the conclusion that users might not really want to have a summary, we could maybe rethink the log/next_step interaction to allow them to skip the summary:

If the user doesn't really want a summary, we could "ask" to always call dvclive.log with a explicit step, otherwise we assume that callingnext_step (either explicitly or implicitly by calling dvclive.log twice without step) means that the user wants a summary.


I know all the above options would imply a significant "shakeup" to the existing API but I think that this kind of discussion has, in the end, the same goal to the one about how to extend the arguments.

@daavoo daavoo added enhancement discussion requires active participation to reach a conclusion maintenance labels Jul 29, 2021
@daavoo
Copy link
Contributor Author

daavoo commented Aug 2, 2021

pin @dberenbaum @pared ☝️


summary could be reused for configuring something like: #124

@pared
Copy link
Contributor

pared commented Aug 2, 2021

html - I agree that we could get rid of that
resume/step - i never used them separately, and I think they could be merged
summary - i think making it part of #124 would be a nice extension of functionality, possibly allowing us to not create it if needed (eg if summary=None dont create it)

I know all the above options would imply a significant "shakeup" to the existing API but I think that this kind of discussion has, in the end, the same goal to the one about how to extend the arguments.

Well, we are at 0.0.6, its still a wild west - we will just bump the minor version in that case.

@dberenbaum
Copy link
Contributor

html

Hmm, yes, it seems like the considerations are similar to iterative/dvc#6182 and iterative/dvc#6332. Agree that it's probably better to have this specified by a dvc env var.

Are these basically hooks for experiments/checkpoints? Maybe we need a more general mechanism here? 🤔

resume/step

They do seem redundant, but how would I do the equivalent of dvclive.init(resume=True) if we collapse them?

@daavoo
Copy link
Contributor Author

daavoo commented Aug 3, 2021

resume/step

They do seem redundant, but how would I do the equivalent of dvclive.init(resume=True) if we collapse them?

resume_from=0. Currently, resume=True is actually step=0, resume=True.

If we define resume_from: Optional[int] = None, we would make:

  • resume_from=None (default) equals to resume=False.
  • resume_from=X equals to resume=True, step=X.
  • resume_from=0 equals to resume=True, step=0, trying to read step from summary.

@daavoo daavoo mentioned this issue Aug 3, 2021
2 tasks
@dberenbaum
Copy link
Contributor

IMHO keeping resume and step separate may be less confusing, or at least it's not an obvious enough improvement to change the API. Something like step=5, resume=True is also confusing, but I think it's unlikely for users to do that unless they are trying to break something 😄 .

@pared
Copy link
Contributor

pared commented Aug 5, 2021

I think that if we would like to have resume_from we would need clear and easy communication equivalent for resume=False and resume=True (which means resume from latest found).

@daavoo
Copy link
Contributor Author

daavoo commented Aug 5, 2021

Maybe combining step and resume is not really the way to go.

Having resume as a boolean makes sense. Maybe we could just remove step from the public init? What would be the scenario where someone passes step=X (being X other than 0) and resum=True)?

If there is such scenario, would it make sense to "hide" the step property under something like dvclive.get_step/dvclive.set_step??

@pared
Copy link
Contributor

pared commented Aug 5, 2021

Having resume as a boolean makes sense. Maybe we could just remove step from the public init? What would be the scenario where someone passes step=X (being X other than 0) and resum=True)?

When one would like to start from a different step than the latest, but it's hard for me to imagine how such code would look like - because the model would also have to be brought back to X.

@dberenbaum
Copy link
Contributor

Removing step from dvclive.init() makes sense to me. And, as mentioned in #113, a step property could be helpful in a lot of scenarios.

@daavoo daavoo mentioned this issue Aug 25, 2021
2 tasks
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 enhancement maintenance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants