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

fix: show selections in ComparisonView on any page (ET-189) #9694

Merged
merged 9 commits into from
Jul 25, 2024
Merged

Conversation

johnkim-det
Copy link
Contributor

@johnkim-det johnkim-det commented Jul 22, 2024

Ticket

ET-189

Description

Even if selected experiments/runs are not loaded on the current page, the Compare panel should still fetch data for them.

Test Plan

On Experiments List page (with "New Experiment List" feature flag on):

  • Select an experiment
  • Open the "Compare" view
  • Change the page
  • Should display "Metrics", "Hyperparameters", and "Details" tabs with appropriate content
    • Should NOT display "No items selected." message

On Runs List page (with "Flat Runs View" feature flag on):

  • Select a run
  • Open the "Compare" view
  • Change the page
  • Should display "Metrics", "Hyperparameters", and "Details" tabs with appropriate content
    • Should NOT display "No items selected." message

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

@johnkim-det johnkim-det requested a review from a team as a code owner July 22, 2024 21:09
@cla-bot cla-bot bot added the cla-signed label Jul 22, 2024
Copy link

netlify bot commented Jul 22, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit a2a436c
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66a295e1c3b73700087c74b3
😎 Deploy Preview https://deploy-preview-9694--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Jul 22, 2024

Codecov Report

Attention: Patch coverage is 89.13858% with 29 lines in your changes missing coverage. Please review.

Project coverage is 48.61%. Comparing base (1500f39) to head (a2a436c).
Report is 37 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9694      +/-   ##
==========================================
- Coverage   53.34%   48.61%   -4.73%     
==========================================
  Files        1255      932     -323     
  Lines      152723   123636   -29087     
  Branches     3246     3249       +3     
==========================================
- Hits        81464    60102   -21362     
+ Misses      71107    63382    -7725     
  Partials      152      152              
Flag Coverage Δ
harness ?
web 51.38% <89.13%> (-0.21%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
.../react/src/components/ComparisonView.test.mock.tsx 99.34% <100.00%> (+0.01%) ⬆️
...components/RunFilterInterstitialModalComponent.tsx 97.58% <100.00%> (-0.64%) ⬇️
...react/src/components/Searches/Searches.settings.ts 100.00% <100.00%> (ø)
webui/react/src/components/Searches/Searches.tsx 56.49% <100.00%> (ø)
...ebui/react/src/pages/FlatRuns/FlatRuns.settings.ts 94.54% <100.00%> (-0.91%) ⬇️
webui/react/src/types.ts 99.69% <100.00%> (+<0.01%) ⬆️
webui/react/src/utils/flatRun.ts 99.24% <100.00%> (+0.55%) ⬆️
webui/react/src/components/CompareMetrics.tsx 15.20% <66.66%> (-80.15%) ⬇️
...t/src/pages/F_ExpList/F_ExperimentList.settings.ts 0.00% <0.00%> (-100.00%) ⬇️
...bui/react/src/pages/F_ExpList/F_ExperimentList.tsx 0.00% <0.00%> (ø)
... and 3 more

... and 323 files with indirect coverage changes

Copy link
Contributor

@ashtonG ashtonG left a comment

Choose a reason for hiding this comment

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

given that the rest of the codebase supports both types of selections, can we do the work to support that here?

selectedExperiments?.length === 0 || selectedRuns?.length === 0 ? (
(selectedExperimentIds?.length && selectedExperiments === undefined) ||
(selectedRunIds?.length && selectedRuns === undefined) ? (
<Spinner />
Copy link
Contributor

Choose a reason for hiding this comment

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

remember to ensure the spinner is spinning.

@johnkim-det johnkim-det requested a review from ashtonG July 23, 2024 18:49
@ashtonG
Copy link
Contributor

ashtonG commented Jul 23, 2024

  • i'm seeing an issue where the colors in the comparison view doesn't match the colors in the table. This can be seen by selecting experiments out of table order.
  • Furthermore, we should probably include messaging when the user has selected more than 50 experiments/runs (in order to handle the select all branch, use the pagination information to determine if more than 50 experiments/runs were in the selection)
  • this wasn't in the ticket, but i'm seeing 431 errors when comparing more than ~20 experiments. this is specifically the searchExperiments endpoint. There's a ticket to switch it to use the post method like we did for searchTrials (ET-602) -- i think we need to block work on this until the other ticket is finished.

@johnkim-det
Copy link
Contributor Author

@ashtonG updated with fix for colorMap and messaging for attempting to fetch more than 50 selections.

@jesse-amano-hpe jesse-amano-hpe added the to-cherry-pick Pull requests that need to be cherry-picked into the current release label Jul 24, 2024
Copy link
Contributor

@ashtonG ashtonG left a comment

Choose a reason for hiding this comment

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

lgtm

webui/react/src/components/ComparisonView.tsx Outdated Show resolved Hide resolved
Comment on lines 60 to 85
const isSelectionLimitReached = () => {
if (experimentSelection) {
if (
(experimentSelection.type === 'ONLY_IN' &&
experimentSelection.selections.length > SELECTION_LIMIT) ||
(experimentSelection.type === 'ALL_EXCEPT' &&
total &&
total - experimentSelection.exclusions.length > SELECTION_LIMIT)
) {
return true;
}
return false;
}
if (runSelection) {
if (
(runSelection.type === 'ONLY_IN' && runSelection.selections.length > SELECTION_LIMIT) ||
(runSelection.type === 'ALL_EXCEPT' &&
total &&
total - runSelection.exclusions.length > SELECTION_LIMIT)
) {
return true;
}
return false;
}
return false;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm wondering if it's worth doing this vs just looking at the pagination information we get back from the endpoint, especially because the selection object could theoretically contain ids that are no longer in the project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, that makes sense, will change this to a state boolean that's set based on pagination.total in searchRuns/searchExperiments response.

@johnkim-det johnkim-det merged commit a32b010 into main Jul 25, 2024
81 of 94 checks passed
@johnkim-det johnkim-det deleted the ET-189 branch July 25, 2024 18:52
github-actions bot pushed a commit that referenced this pull request Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed to-cherry-pick Pull requests that need to be cherry-picked into the current release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants