fix: Bulk Action bug #9255

johnkim-det · 2024-04-26T17:57:44Z

Ticket

ET-237

Description

Removes boolean selectAll and dependent logic from BulkAction/BatchAction handling.

Test Plan

Select multiple rows from the Experiment List
Any action selected from the "Actions" menu that appears should function as expected. Action should not happen on more or less items than what's selected

Checklist

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

codecov · 2024-04-26T17:57:49Z

Codecov Report

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

Project coverage is 44.68%. Comparing base (aea83df) to head (c97f2c1).
Report is 24 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9255      +/-   ##
==========================================
+ Coverage   44.66%   44.68%   +0.02%     
==========================================
  Files        1273     1273              
  Lines      155822   155830       +8     
  Branches     2439     2438       -1     
==========================================
+ Hits        69591    69629      +38     
+ Misses      85992    85962      -30     
  Partials      239      239

Flag	Coverage Δ
backend	`41.75% <53.08%> (+0.01%)`	⬆️
harness	`64.21% <0.00%> (-0.02%)`	⬇️
web	`35.12% <8.40%> (+0.04%)`	⬆️

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

Files	Coverage Δ
master/internal/api_experiment.go	`56.74% <100.00%> (+0.51%)`	⬆️
master/internal/api_runs.go	`73.61% <100.00%> (ø)`
master/internal/core.go	`4.10% <ø> (ø)`
webui/react/src/components/UserSettings.tsx	`85.71% <100.00%> (+2.20%)`	⬆️
...ages/ExperimentDetails/ExperimentDetailsHeader.tsx	`74.96% <100.00%> (+0.03%)`	⬆️
.../react/src/components/ExperimentActionDropdown.tsx	`0.00% <0.00%> (ø)`
webui/react/src/components/ExperimentMoveModal.tsx	`66.07% <50.00%> (-0.30%)`	⬇️
...react/src/components/ExperimentRetainLogsModal.tsx	`59.21% <75.00%> (+0.15%)`	⬆️
webui/react/src/services/types.ts	`0.00% <0.00%> (ø)`
webui/react/src/pages/ExperimentList.tsx	`0.00% <0.00%> (ø)`
... and 8 more

... and 3 files with indirect coverage changes

netlify · 2024-04-26T17:58:02Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`c97f2c1`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/6633c15a53d22600083b17ca
😎 Deploy Preview	https://deploy-preview-9255--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

keita-determined

The ticket says

Disable Infinite Scrolls (+ Remove from experiment view options & user preferences)

i can still enable infinite scroll.

keita-determined

mostly working good. few questions

@johnkim-det when filter is applied, should we reset selection?
cc: @corban-beaird

Screen.Recording.2024-04-26.at.1.39.41.PM.mov

qq:

BulkAction requests to include the relevant project id

where do we supposed to see the projectId? i didnt see it in archive API

corban-beaird · 2024-04-26T20:59:23Z

It looks like the selection count isn't updating properly, the bulk action is properly scope to only the visible experiments, but we're telling users they have 22 out of 2 experiments selected.

Screen.Recording.2024-04-26.at.2.46.15.PM.mov

It also leads to some confusion because it looks like we've selected things, but we don't allow the option to do anything with them. To clarify the problem is not with the lack of an ability to perform actions on the "ghost" selections, but with the ghost selections.

It's also a tad quirky since now we make it seem like they can perform action across multiple pages, but we're not really giving them an option to do so.

corban-beaird · 2024-04-26T21:05:22Z

@keita-determined

qq:

BulkAction requests to include the relevant project id

where do we supposed to see the projectId? i didnt see it in archive API

We're leaving this out as a requirement this is handled by exhaustively listing out the experiment ids.

@johnkim-det
However, the more I'm thinking about this, in the event that an experiment is no longer in the currently open project, then we'd like to make sure don't perform an action on it unintentionally. Assuming another user moved an experiment at the same time and their request was handled first.

So yes, we want to include the project_id, sorry for any confusion!

corban-beaird · 2024-04-30T21:34:19Z

Looks like there's a failure in tests/cluster/test_workspace_org.py::test_workspace_org that isn't occurring on main. There are also some timeouts in other tests, are those flakes?

@MikhailKardash
I was able to replicate tests/cluster/test_workspace_org.py::test_workspace_org on my local if I'm running a licensed instance on latest main w/RBAC enabled; however, the test passes on both this branch & main when it's not.

The issue I'm seeing is that the determined user doesn't have view permission on the Uncategorized workspace when RBAC is enabled which is being assumed.
**Edit: I was able to reproduce the workspace_org issue outside of the RBAC enabled devcluster, working to resolve now
*** Resolved, the bulk action move hadn't account for single experiment request like the others, corrected.

The timeout failures should now be resolved, the batch kill request had not been correctly adjusted originally; it should be now.

ashtonG

is returning infinite scroll on the roadmap? if not, i'd rather us remove the dead code instead of having to work around it indefinitely.

johnkim-det · 2024-05-01T15:11:59Z

my understanding is that infinite scroll should be disabled for now, but ideally want to bring it back if we can find a solution that works well with row selection. I can create a ticket for that so it's captured and we make sure to have that discussion.

maxrussell

Still reviewing, but leaving some comments here to get convo started to hopefully move the PR along faster.

e2e_tests/tests/experiment/experiment.py

master/internal/experiment/bulk_action.go

Co-authored-by: Max Russell <max.russell@hpe.com>

cla-bot · 2024-05-01T17:54:45Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @corban-beaird on file. In order for us to review and merge your code, please start the CLA process at https://determined.ai/cla.

After we approve your CLA, we will update the contributors list (private) and comment @cla-bot[bot] check to rerun the check.

corban-beaird · 2024-05-01T18:48:09Z

@cla-bot[bot] check

cla-bot · 2024-05-01T18:48:13Z

The cla-bot has been summoned, and re-checked this pull request!

ashtonG · 2024-05-01T20:55:10Z

webui/react/src/pages/F_ExpList/F_ExperimentList.tsx

    selection.rows,
    users,
  ]);

  const getHeaderMenuItems = (columnId: string, colIdx: number): MenuItem[] => {
    if (columnId === MULTISELECT) {
      const items: MenuItem[] = [
-        selection.rows.length > 0
+        settings.selection.type === 'ONLY_IN' && settings.selection.selections.length > 0


Suggested change

settings.selection.type === 'ONLY_IN' && settings.selection.selections.length > 0

settings.selection.type === 'ALL_EXCEPT' || settings.selection.selections.length > 0

ashtonG · 2024-05-01T20:56:47Z

webui/react/src/pages/F_ExpList/F_ExperimentList.tsx

+  const allSelectedExperimentIds = useMemo(() => {
+    return settings.selection.type === 'ONLY_IN' ? settings.selection.selections : [];
+  }, [settings.selection]);


move this up to be a dependency of loadedSelectedExperimentIds

maxrussell

Backend portion LGTM

keita-determined · 2024-05-02T09:29:23Z

webui/react/src/components/TableActionBar.tsx

+  const fetchExperimentsByIds = useCallback(async (selectedIds: number[]) => {
+    const CHUNK_SIZE = 80; // match largest pageSizeOption used for Pagination components
+    const chunkedExperimentIds = _.chunk(selectedIds, CHUNK_SIZE);
+    const responses = await Promise.all(


missing try catch

johnkim-det requested a review from a team as a code owner April 26, 2024 17:57

johnkim-det requested a review from julian-determined-ai April 26, 2024 17:57

cla-bot bot added the cla-signed label Apr 26, 2024

johnkim-det requested review from corban-beaird, AmanuelAaron, keita-determined and thiagodallacqua-hpe and removed request for julian-determined-ai April 26, 2024 17:58

keita-determined reviewed Apr 26, 2024

View reviewed changes

johnkim-det requested a review from keita-determined April 26, 2024 19:49

keita-determined reviewed Apr 26, 2024

View reviewed changes

carolinaecalderon added the to-cherry-pick Pull requests that need to be cherry-picked into the current release label Apr 26, 2024

johnkim-det requested a review from a team as a code owner April 29, 2024 18:35

corban-beaird requested a review from keita-determined April 29, 2024 18:58

johnkim-det requested a review from a team as a code owner April 29, 2024 19:12

johnkim-det requested a review from MikhailKardash April 29, 2024 19:12

johnkim-det and others added 8 commits April 29, 2024 13:12

no filters, selectAll, excluded for bulk actions

50673d0

change Select All behavior and remove inf scroll

758a2fa

remove selection obj from TableActionBar

57421aa

chore: adjust bulk experiment endpoints to require project id

3796433

include projectId in BulkAction requests

bf3bcbb

chore: update tests to reflect new required project_id param

8ceefc7

chore: rebase to main

5111d68

chore: clean up retention bulk tests

075dec5

corban-beaird force-pushed the ET-237 branch from 7031d73 to 075dec5 Compare April 29, 2024 19:35

chore: clean up go linter issues

f28c153

chore: correct python linter issues in test_api.py

c2d4186

fix: correct move bulk action

cd24503

ashtonG reviewed May 1, 2024

View reviewed changes

maxrussell reviewed May 1, 2024

View reviewed changes

Update master/internal/experiment/bulk_action.go

98d5a79

Co-authored-by: Max Russell <max.russell@hpe.com>

cla-bot bot removed the cla-signed label May 1, 2024

cla-bot bot added the cla-signed label May 1, 2024

corban-beaird added 2 commits May 1, 2024 14:43

refactor: safer global project id handling

e4517e8

chore: correct rebase issue with function rename

49cdbb4

ashtonG reviewed May 1, 2024

View reviewed changes

corban-beaird added 2 commits May 1, 2024 15:02

refactor: remove default project id from experiment.py

902defd

chore: corrected project id param in test_master_restart.py

004c7d1

maxrussell approved these changes May 1, 2024

View reviewed changes

keita-determined requested a review from MikhailKardash May 1, 2024 21:40

MikhailKardash approved these changes May 1, 2024

View reviewed changes

fix: feedback

8283a71

keita-determined requested a review from ashtonG May 1, 2024 22:56

keita-determined reviewed May 2, 2024

View reviewed changes

johnkim-det added 2 commits May 2, 2024 10:55

try catch for exp fetch

7345579

fixes on runs and searches views

c97f2c1

ashtonG approved these changes May 2, 2024

View reviewed changes

johnkim-det merged commit f6e42cd into main May 2, 2024
86 of 99 checks passed

johnkim-det deleted the ET-237 branch May 2, 2024 17:52

carolinaecalderon pushed a commit that referenced this pull request May 2, 2024

fix: Bulk Action bug (#9255)

e277782

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Bulk Action bug #9255

fix: Bulk Action bug #9255

johnkim-det commented Apr 26, 2024 •

edited by jira bot

Loading

codecov bot commented Apr 26, 2024 •

edited

Loading

netlify bot commented Apr 26, 2024 •

edited

Loading

keita-determined left a comment

keita-determined left a comment

corban-beaird commented Apr 26, 2024

corban-beaird commented Apr 26, 2024 •

edited

Loading

corban-beaird commented Apr 30, 2024 •

edited

Loading

ashtonG left a comment

johnkim-det commented May 1, 2024

maxrussell left a comment

cla-bot bot commented May 1, 2024

corban-beaird commented May 1, 2024

cla-bot bot commented May 1, 2024

ashtonG May 1, 2024 •

edited

Loading

keita-determined May 1, 2024

ashtonG May 1, 2024 •

edited

Loading

keita-determined May 1, 2024

maxrussell left a comment

keita-determined May 2, 2024

	settings.selection.type === 'ONLY_IN' && settings.selection.selections.length > 0
	settings.selection.type === 'ALL_EXCEPT' \|\| settings.selection.selections.length > 0

fix: Bulk Action bug #9255

fix: Bulk Action bug #9255

Conversation

johnkim-det commented Apr 26, 2024 • edited by jira bot Loading

Ticket

Description

Test Plan

Checklist

codecov bot commented Apr 26, 2024 • edited Loading

Codecov Report

netlify bot commented Apr 26, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

keita-determined left a comment

Choose a reason for hiding this comment

keita-determined left a comment

Choose a reason for hiding this comment

corban-beaird commented Apr 26, 2024

corban-beaird commented Apr 26, 2024 • edited Loading

corban-beaird commented Apr 30, 2024 • edited Loading

ashtonG left a comment

Choose a reason for hiding this comment

johnkim-det commented May 1, 2024

maxrussell left a comment

Choose a reason for hiding this comment

cla-bot bot commented May 1, 2024

corban-beaird commented May 1, 2024

cla-bot bot commented May 1, 2024

ashtonG May 1, 2024 • edited Loading

Choose a reason for hiding this comment

keita-determined May 1, 2024

Choose a reason for hiding this comment

ashtonG May 1, 2024 • edited Loading

Choose a reason for hiding this comment

keita-determined May 1, 2024

Choose a reason for hiding this comment

maxrussell left a comment

Choose a reason for hiding this comment

keita-determined May 2, 2024

Choose a reason for hiding this comment

johnkim-det commented Apr 26, 2024 •

edited by jira bot

Loading

codecov bot commented Apr 26, 2024 •

edited

Loading

netlify bot commented Apr 26, 2024 •

edited

Loading

corban-beaird commented Apr 26, 2024 •

edited

Loading

corban-beaird commented Apr 30, 2024 •

edited

Loading

ashtonG May 1, 2024 •

edited

Loading

ashtonG May 1, 2024 •

edited

Loading