Skip to content

exp show: show exp when checkpoint is running(#7329)#7349

Merged
karajan1001 merged 1 commit into
treeverse:mainfrom
karajan1001:fix7329
Feb 15, 2022
Merged

exp show: show exp when checkpoint is running(#7329)#7349
karajan1001 merged 1 commit into
treeverse:mainfrom
karajan1001:fix7329

Conversation

@karajan1001
Copy link
Copy Markdown
Contributor

@karajan1001 karajan1001 commented Feb 4, 2022

Fixes #7329

In a previous PR, we introduce a bug which use HEAD as the workspace
commit, while if there is a checkpoint is running, the HEAD is
actually changing all the time, we should use
refs/exps/exec/EXEC_BASELINE instead.

A test is needed for this PR.

  1. replace the default HEAD in exp show with fix_exp_head.
  2. modify a functional test to simulate the running conditions.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@karajan1001 karajan1001 requested a review from a team as a code owner February 4, 2022 10:43
@karajan1001 karajan1001 requested review from efiop and pmrowla and removed request for efiop February 4, 2022 10:43
@karajan1001 karajan1001 changed the title [WIP] exp show: show exp when checkpoint is running(#7329) exp show: show exp when checkpoint is running(#7329) Feb 4, 2022
Copy link
Copy Markdown
Contributor

@pmrowla pmrowla left a comment

Choose a reason for hiding this comment

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

So the actual cause of this bug is that we don't handle fixing HEAD in iter_revs:

https://github.com/iterative/dvc/blob/e430693fc81b0ba27131756039e3191d0b93d83d/dvc/scm.py#L157

I think what we should really do for this is to make dvc.scm.resolve_rev() use fix_exp_head, and get rid of all of the other calls to fix_exp_head from elsewhere in DVC, otherwise we will probably keep running into this type of bug in the future.

and then we can just have one test for scm.resolve_rev() that does the same thing as:

https://github.com/iterative/dvc/blob/e430693fc81b0ba27131756039e3191d0b93d83d/tests/func/experiments/test_experiments.py#L625-L636

At some point what we really want is to make scm.resolve_rev() use fix_exp_head, and get rid of all of the other fix_exp_head calls everywhere else in DVC,

@karajan1001
Copy link
Copy Markdown
Contributor Author

Currently, scmrepo is decoupled with this experiments related setting? Maybe it is better to put this logic in dvc/repo/experiments/utils.py instead of scmrepo. BTW, the import logic of dvc/repo/experiments/utils.py and dvc/scm.py is a little bit tricky, sometimes utils.py depends on something from scm.py sometimes the dependency comes from the opposite direction.

And besides this, I don't know if there are any conditions that we need to resolve the current temp local HEAD instead of the exp_head?

@pmrowla
Copy link
Copy Markdown
Contributor

pmrowla commented Feb 8, 2022

Currently, scmrepo is decoupled with this experiments related setting? Maybe it is better to put this logic in dvc/repo/experiments/utils.py instead of scmrepo. BTW, the import logic of dvc/repo/experiments/utils.py and dvc/scm.py is a little bit tricky, sometimes utils.py depends on something from scm.py sometimes the dependency comes from the opposite direction.

This does not belong in scmrepo for sure, dvc.scm is for scm/git behavior that is DVC specific (like this issue).

The main issue with keeping fix_exp_head separate from dvc.scm.resolve_rev is that this affects every DVC command (such asdvc diff), not just exp commands.

And besides this, I don't know if there are any conditions that we need to resolve the current temp local HEAD instead of the exp_head?

In the event that we need the local head, we can just use scm.get_ref("HEAD") directly.

@karajan1001
Copy link
Copy Markdown
Contributor Author

karajan1001 commented Feb 9, 2022

OK dvc.scm.resolve_rev (dvc/scm.py::resolve_rev) is different from dvc.scm.resolve_rev (dvc("Repo").scm("Git")::resolve_rev) quite confusing.

I would suggest renaming it here to make them more clarified.

fix: treeverse#7329
In a previous PR, we introduce a bug which use `HEAD` as the workspace
commit, while if there is a checkpoint is running, the `HEAD` is
actually changing all the time, we should use
`refs/exps/exec/EXEC_BASELINE` instead.

1. add `fix_exp_head` transfer in `dvc/scm.py::resolve_rev`
2. remove fix_exp_head from `diff`, `metrics diff`, `params diff` and
   `plots diff`
3. modify a functional test to simulate the running conditions
@karajan1001 karajan1001 merged commit d1f7cf4 into treeverse:main Feb 15, 2022
@karajan1001 karajan1001 deleted the fix7329 branch February 16, 2022 04:05
@efiop efiop added the bugfix fixes bug label Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix fixes bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

exp show: checkpoint data is no longer returned when running checkpoints based experiment

3 participants