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

example-dvc-experiments: Added modifications for DVCLive #95

Closed
wants to merge 18 commits into from

Conversation

iesahin
Copy link
Contributor

@iesahin iesahin commented Nov 9, 2021

  • Modifies train.py to use DvcLiveKerasCallback. Adds dvclive<0.5 to the requirements.txt. These changes are added as the final tag.
  • Modifies to run 3 baseline experiments (instead of 4) and adds another 3 to run with the live modifications.

Closes #81

example-dvc-experiments/code-dvclive/train.py Outdated Show resolved Hide resolved
example-dvc-experiments/code-dvclive/train.py Outdated Show resolved Hide resolved
example-dvc-experiments/generate.bash Outdated Show resolved Hide resolved
example-dvc-experiments/code-dvclive/train.py Outdated Show resolved Hide resolved
@iesahin
Copy link
Contributor Author

iesahin commented Nov 10, 2021

In the light of changes required, do you think it's best to put these into example-dvc-experiments repository? @daavoo Could we put to another branch? I tried to make minimum changes but this one will be the final state of the repository. These changes will make "dvc exp w/o dvclive" useless. It will be like DVCLive is required to run the experiments.

WDYT @dberenbaum @shcheklein

@shcheklein
Copy link
Member

I'm personally fine to depend on dvclive. I would though put a comment in code that let's say "dvclive.json" is the same (or could be replaced with):

with open ...
    f.write(...

and dvclive plays a role of a lighthweight helper to write metric files.

@dberenbaum
Copy link

These changes will make "dvc exp w/o dvclive" useless.

I'm not sure what you mean and what your concerns are. Is it that dvclive becomes a dependency of the project? Is it that the project structure has to change? Something else?

@iesahin
Copy link
Contributor Author

iesahin commented Nov 15, 2021

I'm not sure what you mean and what your concerns are. Is it that dvclive becomes a dependency of the project? Is it that the project structure has to change? Something else?

Someone that git clone https://.../example-dvc-experiments will be using a dvclive version instead of a basic dvc exp version. This may look like the experiments need dvclive to run. That's not an important concern actually, and I think merging the live features into DVC may be a better strategy in the longer term for both dvc exp and live.

@dberenbaum
Copy link

I agree with @shcheklein that it's fine to start with dvclive since this is the "happy path" as long as we can make clear that this is a convenience rather than a necessity. We might want to include dvclive as a dvc dependency so that users don't need to install multiple packages.

@iesahin
Copy link
Contributor Author

iesahin commented Nov 16, 2021

I'm updating the project to have DVCLive from the start, and all related functionality will use DVCLive, not custom metrics/plots. @shcheklein @dberenbaum

@iesahin iesahin self-assigned this Nov 17, 2021
@iesahin
Copy link
Contributor Author

iesahin commented Dec 8, 2021

Now we can make a decision between this (using dvclive) and #97 (using dvc exp init). There is also a third option not to change anything from the example projects and use master as is.

WDYT? @shcheklein @dberenbaum @daavoo @jorgeorpinel

@daavoo
Copy link
Contributor

daavoo commented Dec 8, 2021

Now we can make a decision between this (using dvclive) and #97 (using dvc exp init). There is also a third option not to change anything from the example projects and use master as is.

WDYT? @shcheklein @dberenbaum @daavoo @jorgeorpinel

dvclive and dvc exp init should not be incompatible so, why not both?

dvc exp init --type dl would enable dvclive

@dberenbaum
Copy link

dvc exp init --type dl would enable dvclive

I'm not sure that's the best intro here since it also introduces checkpoints, which create a different workflow. We could use dvclive without --type dl, either using dvc exp init --live somepath to use the live section in dvc.yaml or by having the metrics and/or plots sections of dvc.yaml align with the dvclive outputs. If we do this, I think we need to make clear somewhere how users can do this without dvclive, or at least without a built-in callback.

@daavoo
Copy link
Contributor

daavoo commented Dec 9, 2021

dvc exp init --type dl would enable dvclive

I'm not sure that's the best intro here since it also introduces checkpoints, which create a different workflow. We could use dvclive without --type dl, either using dvc exp init --live somepath to use the live section in dvc.yaml or by having the metrics and/or plots sections of dvc.yaml align with the dvclive outputs. If we do this, I think we need to make clear somewhere how users can do this without dvclive, or at least without a built-in callback.

It seems that the --live flag only has an effect if --type=dl:

https://github.com/iterative/dvc/blob/82c5caee27d4b5591d4ab0b07fd1a73064ba8bff/dvc/repo/experiments/init.py#L218

@dberenbaum
Copy link

It seems that the --live flag only has an effect if --type=dl:

The intended behavior is a bit unclear, but dvc exp init --live somepath will include the live section. Regardless, this might be confusing. I'd vote to use the normal metrics and/or plots sections but generate them through dvclive calls. That's what I ended up doing for the dvc exp init demo.

@iesahin
Copy link
Contributor Author

iesahin commented Dec 13, 2021

I replace initial dvc stage add command with a similar dvc exp init, without changing the flow of repo generation. However, I'm getting:

ERROR: failed to reproduce 'dvc.yaml': Checkpoint stages are not supported in 'dvc repro'. Checkpoint stages must be reproduced with 'dvc exp run' or 'dvc exp resume'.

Now, we have iterative/dvc#6592 that leads to WARNINGs to appear for users regarding the model file, and I began to use dvc repro for the first experiment to prevent them. It looks now we are at a point to select either dvc exp init with these warnings, or dvc repro without dvc exp init.

I made some simplifications in the text, in iterative/dvc.org#3051 but I still believe, we can postpone the major update until these issues are resolved. @dberenbaum @shcheklein

@dberenbaum
Copy link

@iesahin I'm fine if you think it's not worthwhile to include exp init and/or dvclive yet in the get started doc.

With regards to the error you mention, it seems like you have checkpoint: true somewhere in your dvc.yaml, which I thought from the discussion above was out of scope.

@iesahin
Copy link
Contributor Author

iesahin commented Dec 17, 2021

closed in favor of #97

@iesahin iesahin closed this Dec 17, 2021
@shcheklein shcheklein deleted the iesahin/add-dvclive-to-exp branch April 22, 2022 22:57
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.

Create an example project for dvclive separate from example-dvc-checkpoints.
4 participants