determined icon indicating copy to clipboard operation
determined copied to clipboard

fix: actually pass search filters to experiment actions

Open ashtonG opened this issue 10 months ago • 5 comments

Description

This fixes an issue where bulk experiment actions did not work when using select all. The endpoint used a different, incompatible filter format that did not apply the proper filters to the selection. We introduce the new filter as a new parameter (for backwards compatibility's sake) and apply it to all actions.

Test Plan

one

  • On an experiment list page, select 2 experiments, click the actions menu and then click "archive"
    • the selected experiments should be archived (if the current view does not show archived experiments, they should be hidden)
  • in the filters menu, select "Show archived", then click the selection column header, then click "select all"
  • click the actions menu and select "unarchive"
  • confirm the action
  • The experiment list should show the same experiments regardless of the "show archived" filter state

two

  • submit an unmanaged experiment to a project with 20 or more experiments in it with a name beginning with the letter a
  • to the same project, submit a managed experiment with a name beginning with the letter z
  • on an experiment list of that project, disable infinite pagination
  • on page one of the experiment list, select the unmanaged experiment
  • on the last page of the experiment list, select the managed experiment
  • The ui should show that you have two experiments selected
  • click the actions menu and select "pause"
  • confirm the action
  • There should be a popup saying that one experiment was paused

Checklist

  • [x] Changes have been manually QA'd
  • [x] User-facing API changes need the "User-facing API Change" label.
  • [x] Release notes should be added as a separate file under docs/release-notes/. See Release Note for details.
  • [x] Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

ET-33

ashtonG avatar Apr 01 '24 21:04 ashtonG

Deploy Preview for determined-ui canceled.

Name Link
Latest commit 76f65e2abdb5475649ceaab4aeeeaee6fb1d0e33
Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/662ad65daae7b00008784347

netlify[bot] avatar Apr 01 '24 21:04 netlify[bot]

Codecov Report

Attention: Patch coverage is 16.19537% with 326 lines in your changes are missing coverage. Please review.

Project coverage is 44.69%. Comparing base (1e87778) to head (76f65e2). Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9083      +/-   ##
==========================================
- Coverage   44.71%   44.69%   -0.03%     
==========================================
  Files        1270     1270              
  Lines      155328   155438     +110     
  Branches     2436     2438       +2     
==========================================
+ Hits        69461    69474      +13     
- Misses      85631    85728      +97     
  Partials      236      236              
Flag Coverage Δ
backend 41.62% <21.83%> (-0.03%) :arrow_down:
harness 64.19% <14.28%> (-0.09%) :arrow_down:
web 35.21% <10.34%> (-0.01%) :arrow_down:

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

Files Coverage Δ
...omponents/FilterForm/components/FilterFormStore.ts 93.68% <100.00%> (ø)
master/internal/api_experiment.go 56.34% <96.55%> (+0.11%) :arrow_up:
master/internal/api_runs.go 70.20% <80.00%> (ø)
master/internal/filter/experiment_filter.go 95.14% <83.33%> (ø)
webui/react/src/components/Searches/Searches.tsx 0.00% <0.00%> (ø)
webui/react/src/components/ExperimentMoveModal.tsx 67.28% <50.00%> (+0.91%) :arrow_up:
...react/src/components/ExperimentRetainLogsModal.tsx 60.13% <60.00%> (+1.07%) :arrow_up:
webui/react/src/services/types.ts 0.00% <0.00%> (ø)
master/internal/api_project.go 24.16% <0.00%> (ø)
...eact/src/components/ExperimentTensorBoardModal.tsx 0.00% <0.00%> (ø)
... and 5 more

... and 3 files with indirect coverage changes

codecov[bot] avatar Apr 02 '24 20:04 codecov[bot]

@keita-determined updated the pr with a fix for the selection spanning across pages and added a test case that should handle most edge cases.

ashtonG avatar Apr 17 '24 21:04 ashtonG

I'm not entirely clear on why we need to maintain the old filter parameter and struct for these bulk endpoints, do we have other things using these outside of our WebUI?

corban-beaird avatar Apr 25 '24 19:04 corban-beaird

For the scope of this ticket from a triage perspective, I can understand why we'd leave both filter parameters in; however, for the sake of code hygiene, I'd like to come back and re-evaluate how we approach this problem, by at least removing the need for one or the other parameter, and I'd be curious if we could get to a position where we could eliminate the need for specifying the filters entirely (only allowing actions to be done on a per page level).

Additionally, our bulk select feature feels odd. We provide options to select the first 5, 10, 25, or all experiments; however, our page sizes are restricted to 20, 40, and 80. Not a single option allows selecting an entire page. Why would our bulk select not first select everything in the page and then give the object to select everything in the given project with the filters applied.

corban-beaird avatar Apr 25 '24 20:04 corban-beaird

closing -- we can circle back to this later.

ashtonG avatar Jun 05 '24 19:06 ashtonG