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

Show experiment names in comparison table headers (#1614) #1730

Merged
merged 5 commits into from
May 20, 2022

Conversation

wolmir
Copy link
Contributor

@wolmir wolmir commented May 17, 2022

This PR will show the displayNameOrParent property of the selected experiment revisions in the comparison table webview when they are available.

Screen Shot 2022-05-17 at 12 48 19

Issue

@wolmir wolmir requested a review from a team May 17, 2022 17:46
@wolmir wolmir self-assigned this May 17, 2022
@rogermparent
Copy link
Contributor

I noticed in the All Small / 1280px Chromatic story that the title text can be line-broken in some situations

image

Is this something we're okay with, or do we want to do something like truncation to make sure the headers don't get expanded?

@mattseddon mattseddon added the product PR that affects product label May 17, 2022
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.

Let's merge this and follow up on the improvements mentioned.

  • crop title
  • remove commented code
  • add extra field for checkpoints

.experimentName {
color: $meta-cell-color;
// font-size: 0.8em;
// line-height: 0;
Copy link
Member

Choose a reason for hiding this comment

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

[I] Let's delete these if we aren't using them.

@@ -177,8 +177,9 @@ export class PlotsModel extends ModelWithPersistence {
this.comparisonOrder,
this.experiments
.getSelectedRevisions()
.map(({ label: revision, displayColor }) => ({
.map(({ label: revision, displayColor, displayNameOrParent }) => ({
Copy link
Member

Choose a reason for hiding this comment

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

[C] I asked the question in the ticket as to whether this will be enough (it doesn't provide the name of the experiment if a checkpoint is not the checkpoint_tip i.e the last iteration). We'll need to follow up and add another field to provide the extra information where it is needed.

I think that we will need to display it whenever the checkpoint is shown outside of the logical group. For example in the comparison table. We will also need it when we come to add the new header section in the plots webview:

@wolmir
Copy link
Contributor Author

wolmir commented May 20, 2022

@shcheklein @mattseddon
Regarding this specific case: https://dvc.org/doc/user-guide/experiment-management/checkpoints#starting-from-an-existing-checkpoint

What should be shown in the ribbons? The root name ([exp-3b-52b] in the example), or the resumed checkpoint short sha (963b396 in the example)?

@wolmir wolmir force-pushed the show-experiment-name-in-table-header branch from 5aed172 to 9c83f25 Compare May 20, 2022 17:53
@wolmir wolmir force-pushed the show-experiment-name-in-table-header branch from e5a05e5 to 1f77030 Compare May 20, 2022 19:10
@codeclimate
Copy link

codeclimate bot commented May 20, 2022

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

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

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

View more on Code Climate.

@wolmir wolmir merged commit 5fdbe92 into main May 20, 2022
@wolmir wolmir deleted the show-experiment-name-in-table-header branch May 20, 2022 19:31
@mattseddon
Copy link
Member

mattseddon commented May 20, 2022

@shcheklein @mattseddon Regarding this specific case: https://dvc.org/doc/user-guide/experiment-management/checkpoints#starting-from-an-existing-checkpoint

What should be shown in the ribbons? The root name ([exp-3b-52b] in the example), or the resumed checkpoint short sha (963b396 in the example)?

image

For this particular example, my suggestion would be to leave the link to the previous experiment in the table/tree view and use the current experiment name for the plots view (i.e show [exp-3b52b]). Either way is fine though.


// if (!experiment) {
// return
// }
Copy link
Member

Choose a reason for hiding this comment

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

[Q] What happened here?

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