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

Multiple commits in the experiments table #2392

Merged
merged 15 commits into from
Sep 23, 2022
Merged

Multiple commits in the experiments table #2392

merged 15 commits into from
Sep 23, 2022

Conversation

sroy3
Copy link
Contributor

@sroy3 sroy3 commented Sep 13, 2022

Part of #1966
Screen Shot 2022-09-13 at 2 23 18 PM
Quite straight forward. Props to @mattseddon for thinking ahead, because this just works.

@sroy3 sroy3 added the product PR that affects product label Sep 13, 2022
@sroy3 sroy3 self-assigned this Sep 13, 2022
@sroy3
Copy link
Contributor Author

sroy3 commented Sep 14, 2022

Screen.Recording.2022-09-14.at.2.33.55.PM.mov

Doing some tests, haven't found anything "weird" yet.

Copy link
Member

@mattseddon mattseddon left a comment

Choose a reason for hiding this comment

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

Initially, I was very confused by the chevron placement of the second commit but it does seem like everything is working as expected:

Screen Shot 2022-09-15 at 4 35 13 pm (2)

I do think we now need to nest experiments underneath their respective commit in the experiments tree:

Screen Shot 2022-09-15 at 4 38 51 pm (2)

The top of the tree is flat and fairly confusing. We should follow up and make that change ASAP.

There is at least one function (collectWorkspaceRunningCheckpoint) which is (probably) checking previously commits when it doesn't need to be.

We should also add a small multi-commit test fixture 🙏

@mattseddon
Copy link
Member

mattseddon commented Sep 15, 2022

Unable to apply an experiment from a previous commit:

Screen.Recording.2022-09-15.at.4.54.44.pm.mov

This doesn't work for either the commit/short sha or the name. It looks like exp apply will only accept names from the current commit. That means we need to raise a bug in DVC and suppress the option from the tree/palette/table for previous commits (for the time being).

 ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
  Experiment                Created        State    avg_prec   roc_auc   prepare.split   prepare.seed   featurize.max_features   featurize.ngrams   train.seed   train.n_est   train.min_
 ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
  workspace                 -              -         0.92438   0.94473   0.21            20170428       200                      2                  20170428     50            0.01      
  test-multi                04:29 PM       -           0.925   0.94602   0.2             20170428       200                      2                  20170428     50            0.01      
  ├── a7c62f2 [exp-1e491]   04:34 PM       -         0.92438   0.94473   0.21            20170428       200                      2                  20170428     50            0.01      
  └── 397549b               04:29 PM       Queued          -         -   0.2             20170428       200                      2                  20170428     50            0.01      
  bigrams-experiment        May 20, 2022   -           0.925   0.94602   0.2             20170428       200                      2                  20170428     50            0.01      
  ├── 15d24d7 [exp-df115]   Aug 16, 2022   -         0.94254   0.96158   0.2             20170428       448                      2                  20170428     50            0.01      
  ├── 58f48c7 [exp-869d9]   Aug 16, 2022   -         0.94489   0.96199   0.2             20170428       512                      2                  20170428     50            0.01      
  ├── 806879b [exp-41525]   Aug 16, 2022   -         0.94559   0.96137   0.2             20170428       576                      2                  20170428     50            0.01      
  ├── 8e112e6 [exp-53de3]   Aug 16, 2022   -         0.94679   0.96299   0.2             20170428       640                      2                  20170428     50            0.01      
  ├── d4e2429 [exp-c37c1]   Aug 16, 2022   -         0.84277   0.89615   0.2             20170428       64                       2                  20170428     50            0.01      
  ├── 62b8ff1 [exp-f5669]   Aug 16, 2022   -         0.94144   0.95794   0.2             20170428       320                      2                  20170428     50            0.01      
  └── cce158f [exp-44136]   Jul 19, 2022   -               -         -   0.2             20170428       200                      2                  20170428     50            0.01      
 ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
~/p/example-get-started test-multi *1 !7 ❯ dvc exp apply 58f48c7
ERROR: '58f48c7' does not appear to be an experiment commit.: Experiment derived from 'a470fae', expected 'dc91a3d'.
~/p/example-get-started test-multi *1 !7 ❯ dvc exp apply exp-869d9
ERROR: 'exp-869d9' does not appear to be an experiment commit.: Experiment derived from 'a470fae', expected 'dc91a3d'.```

Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Great work! Agreed with Matt comments. I was also confused about the placement of the commit rows since they're at the same level as the main branch.

@@ -28,9 +28,11 @@ export enum SubCommand {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the row borders are now a bit buggy which I believe is due to this line in our styles:

Screen Shot 2022-09-15 at 7 43 29 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly, the CSS lines were added to make the last row on the table not have a border 🤔

@sroy3 sroy3 marked this pull request as draft September 15, 2022 22:47
@sroy3 sroy3 marked this pull request as ready for review September 22, 2022 19:23
@sroy3 sroy3 marked this pull request as draft September 23, 2022 15:38
@sroy3 sroy3 marked this pull request as ready for review September 23, 2022 19:27
@codeclimate
Copy link

codeclimate bot commented Sep 23, 2022

Code Climate has analyzed commit d8b04fb and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 91.3% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.9% (0.0% change).

View more on Code Climate.

@sroy3 sroy3 merged commit ccd6cab into main Sep 23, 2022
@sroy3 sroy3 deleted the multiple-commits branch September 23, 2022 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants