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

checkpoints/live: move execution env to stage #5257

Merged
merged 2 commits into from
Jan 18, 2021

Conversation

pared
Copy link
Contributor

@pared pared commented Jan 13, 2021

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

This PR aims to make Output source of the execution environment for Stage.run. It seems that outputs are the cause of env.
Related: #4902 (comment)

@pared pared requested a review from pmrowla January 13, 2021 00:31
@pmrowla
Copy link
Contributor

pmrowla commented Jan 13, 2021

This looks ok to me with the way checkpoints and dvclive are currently defined, but there has been some discussion on whether or not checkpoint should just be a stage level flag in the future, rather than an output level flag. Not sure if the same consideration would apply to dvclive?

@pared pared added refactoring Factoring and re-factoring skip-changelog Skips changelog labels Jan 13, 2021
@pared
Copy link
Contributor Author

pared commented Jan 13, 2021

@pmrowla
Well, as of now, there can be only one dvclive defined. And env is resource used mainly by stage, so it does make sense to me to define it on stage level. I can move env out of out into stage. How about that?

@pmrowla
Copy link
Contributor

pmrowla commented Jan 13, 2021

@pmrowla
Well, as of now, there can be only one dvclive defined. And env is resource used mainly by stage, so it does make sense to me to define it on stage level. I can move env out of out into stage. How about that?

I think having it in stage makes more sense πŸ‘

@pared
Copy link
Contributor Author

pared commented Jan 13, 2021

@pmrowla moved env to stage

@efiop
Copy link
Contributor

efiop commented Jan 13, 2021

@pared Looks like PR and commit titles need an update too?

@pared pared changed the title checkpoints: move execution env to Output checkpoints/live: move execution env to stage Jan 13, 2021
elif out.checkpoint and checkpoint_func:
from dvc.env import DVC_CHECKPOINT, DVC_ROOT

env.update({DVC_CHECKPOINT: "1", DVC_ROOT: self.repo.root_dir})
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't because of your changes, but looking at this now, I think we should be setting DVC_ROOT all the time in the calling env() function. (DVC_CHECKPOINT should still only be set inside this if case)

For experiments that are run in temp-dirs (and not only checkpoint experiments), it will always be useful for user's code to be able to see where it is being run, since they may need to work with relpaths from that directory (instead of relpaths inside their git/dvc repo workspace). And outside of the experiment context, it won't hurt us to set DVC_ROOT for regular repro runs as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

@pared pared requested a review from pmrowla January 15, 2021 12:10
@efiop efiop merged commit 28f6bea into iterative:master Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Factoring and re-factoring skip-changelog Skips changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants