fix: feedback

determined-ai · Jul 9, 2024 · 655c7f4 · 655c7f4
1 parent 77b99e2
commit 655c7f4
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 21 deletions.
diff --git a/webui/react/src/components/RunActionDropdown.test.tsx b/webui/react/src/components/RunActionDropdown.test.tsx
@@ -4,7 +4,14 @@ import UIProvider, { DefaultTheme } from 'hew/Theme';
 import { ConfirmationProvider } from 'hew/useConfirm';
 
 import { handlePath } from 'routes/utils';
-import { archiveRuns, deleteRuns, killRuns, unarchiveRuns } from 'services/api';
+import {
+  archiveRuns,
+  deleteRuns,
+  killRuns,
+  pauseRuns,
+  resumeRuns,
+  unarchiveRuns,
+} from 'services/api';
 import { FlatRunExperiment, RunState } from 'types';
 
 import RunActionDropdown, { Action } from './RunActionDropdown';
@@ -30,6 +37,8 @@ vi.mock('services/api', () => ({
   archiveRuns: vi.fn(),
   deleteRuns: vi.fn(),
   killRuns: vi.fn(),
+  pauseRuns: vi.fn(),
+  resumeRuns: vi.fn(),
   unarchiveRuns: vi.fn(),
 }));
 
@@ -183,7 +192,7 @@ describe('RunActionDropdown', () => {
     expect(screen.queryByText(Action.Move)).not.toBeInTheDocument();
   });
 
-  it('should provide Pause option', () => {
+  it('should provide Pause option', async () => {
     mocks.canModifyFlatRun.mockImplementation(() => true);
     const experiment: FlatRunExperiment = {
       description: '',
@@ -199,6 +208,9 @@ describe('RunActionDropdown', () => {
     };
     setup(undefined, RunState.Active, false, experiment);
     expect(screen.getByText(Action.Pause)).toBeInTheDocument();
+    await user.click(screen.getByText(Action.Pause));
+    await user.click(screen.getByRole('button', { name: Action.Pause }));
+    expect(vi.mocked(pauseRuns)).toBeCalled();
   });
 
   it('should hide Pause option without permissions', () => {
@@ -207,10 +219,13 @@ describe('RunActionDropdown', () => {
     expect(screen.queryByText(Action.Pause)).not.toBeInTheDocument();
   });
 
-  it('should provide Resume option', () => {
+  it('should provide Resume option', async () => {
     mocks.canModifyFlatRun.mockImplementation(() => true);
     setup(undefined, RunState.Paused, false);
     expect(screen.getByText(Action.Resume)).toBeInTheDocument();
+    await user.click(screen.getByText(Action.Resume));
+    await user.click(screen.getByRole('button', { name: Action.Resume }));
+    expect(vi.mocked(resumeRuns)).toBeCalled();
   });
 
   it('should hide Resume option without permissions', () => {

diff --git a/webui/react/src/components/RunActionDropdown.tsx b/webui/react/src/components/RunActionDropdown.tsx
@@ -182,7 +182,7 @@ const RunActionDropdown: React.FC<Props> = ({
       } catch (e) {
         handleError(e, {
           level: ErrorLevel.Error,
-          publicMessage: `Unable to ${action} experiment ${run.id}.`,
+          publicMessage: `Unable to ${action} run ${run.id}.`,
           publicSubject: `${capitalize(action)} failed.`,
           silent: false,
           type: ErrorType.Server,

diff --git a/webui/react/src/pages/FlatRuns/FlatRunActionButton.tsx b/webui/react/src/pages/FlatRuns/FlatRunActionButton.tsx
@@ -25,7 +25,7 @@ import projectStore from 'stores/projects';
 import { BulkActionResult, ExperimentAction, FlatRun, Project } from 'types';
 import handleError from 'utils/error';
 import { canActionFlatRun, getActionsForFlatRunsUnion } from 'utils/flatRun';
-import { capitalizeWord } from 'utils/string';
+import { capitalizeWord, pluralizer } from 'utils/string';
 
 const BATCH_ACTIONS = [
   ExperimentAction.Move,
@@ -127,24 +127,20 @@ const FlatRunActionButton = ({
         } else if (numFailures === 0) {
           openToast({
             closeable: true,
-            description: `${action} succeeded for ${
-              results.successful.length
-            } ${LABEL_PLURAL.toLowerCase()}`,
+            description: `${action} succeeded for ${results.successful.length} ${pluralizer(results.successful.length, 'run')}`,
             title: `${action} Success`,
           });
         } else if (numSuccesses === 0) {
           openToast({
-            description: `Unable to ${action.toLowerCase()} ${numFailures} ${LABEL_PLURAL.toLowerCase()}`,
+            description: `Unable to ${action.toLowerCase()} ${numFailures} ${pluralizer(numFailures, 'run')}`,
             severity: 'Warning',
             title: `${action} Failure`,
           });
         } else {
           openToast({
             closeable: true,
-            description: `${action} succeeded for ${numSuccesses} out of ${
-              numFailures + numSuccesses
-            } eligible
-            ${LABEL_PLURAL.toLowerCase()}`,
+            description: `${action} succeeded for ${numSuccesses} out of ${numFailures + numSuccesses} eligible
+            ${pluralizer(numFailures + numSuccesses, 'run')}`,
             severity: 'Warning',
             title: `Partial ${action} Failure`,
           });
@@ -219,20 +215,20 @@ const FlatRunActionButton = ({
       } else if (numFailures === 0) {
         openToast({
           closeable: true,
-          description: `${results.successful.length} runs moved to project ${destinationProjectName}`,
+          description: `${results.successful.length} ${pluralizer(results.successful.length, 'run')} moved to project ${destinationProjectName}`,
           link: <Link path={paths.projectDetails(destinationProjectId)}>View Project</Link>,
           title: 'Move Success',
         });
       } else if (numSuccesses === 0) {
         openToast({
-          description: `Unable to move ${numFailures} runs`,
+          description: `Unable to move ${numFailures} ${pluralizer(numFailures, 'run')}`,
           severity: 'Warning',
           title: 'Move Failure',
         });
       } else {
         openToast({
           closeable: true,
-          description: `${numFailures} out of ${numFailures + numSuccesses} eligible runs failed to move to project ${destinationProjectName}`,
+          description: `${numFailures} out of ${numFailures + numSuccesses} eligible ${pluralizer(numFailures + numSuccesses, 'run')} failed to move to project ${destinationProjectName}`,
           link: <Link path={paths.projectDetails(destinationProjectId)}>View Project</Link>,
           severity: 'Warning',
           title: 'Partial Move Failure',

diff --git a/webui/react/src/utils/flatRun.test.ts b/webui/react/src/utils/flatRun.test.ts
@@ -226,12 +226,33 @@ describe('Flat Run Utilities', () => {
           };
           expect(canActionFlatRun(FlatRunAction.Pause, flatRun)).toBeTruthy();
 
+          const flatRunWithSingletrial: FlatRun = {
+            ...flatRun,
+            experiment: {
+              description: '',
+              id: 1,
+              isMultitrial: false,
+              name: 'name',
+              progress: 1,
+              resourcePool: 'default',
+              searcherMetric: 'validation_loss',
+              searcherType: 'adaptive_asha',
+              unmanaged: false,
+            },
+          };
+          expect(canActionFlatRun(FlatRunAction.Pause, flatRunWithSingletrial)).toBeTruthy();
+        },
+      );
+
+      it.each(Object.values(RunState).filter((v) => pausableRunStates.has(v)))(
+        'should not be pausable with pausable run states (%s)',
+        (runState) => {
           const flatRunWithMultitrial: FlatRun = {
             ...BASE_FLAT_RUN,
             experiment: {
               description: '',
               id: 1,
-              isMultitrial: false,
+              isMultitrial: true,
               name: 'name',
               progress: 1,
               resourcePool: 'default',
@@ -241,21 +262,37 @@ describe('Flat Run Utilities', () => {
             },
             state: runState,
           };
-          expect(canActionFlatRun(FlatRunAction.Pause, flatRunWithMultitrial)).toBeTruthy();
+          expect(canActionFlatRun(FlatRunAction.Pause, flatRunWithMultitrial)).toBeFalsy();
         },
       );
 
       it.each(Object.values(RunState).filter((v) => !pausableRunStates.has(v)))(
-        'should not be pausable (%s)',
+        'should not be pausable with nonpausable run states (%s)',
         (runState) => {
           const flatRun: FlatRun = {
             ...BASE_FLAT_RUN,
             state: runState,
           };
           expect(canActionFlatRun(FlatRunAction.Pause, flatRun)).toBeFalsy();
 
+          const flatRunWithSingletrial: FlatRun = {
+            ...flatRun,
+            experiment: {
+              description: '',
+              id: 1,
+              isMultitrial: false,
+              name: 'name',
+              progress: 1,
+              resourcePool: 'default',
+              searcherMetric: 'validation_loss',
+              searcherType: 'single',
+              unmanaged: false,
+            },
+          };
+          expect(canActionFlatRun(FlatRunAction.Pause, flatRunWithSingletrial)).toBeFalsy();
+
           const flatRunWithMultitrial: FlatRun = {
-            ...BASE_FLAT_RUN,
+            ...flatRun,
             experiment: {
               description: '',
               id: 1,
@@ -267,7 +304,6 @@ describe('Flat Run Utilities', () => {
               searcherType: 'adaptive_asha',
               unmanaged: false,
             },
-            state: runState,
           };
           expect(canActionFlatRun(FlatRunAction.Pause, flatRunWithMultitrial)).toBeFalsy();
         },