chore: update copy when f_flat_runs is on #9642

ashtonG · 2024-07-11T19:52:34Z

Ticket

Description

This updates the copy in the app when the flat runs feature is on. Places accessible in the app when flat runs is on should refer to searches and runs instead of experiments and trials.

Test Plan

when viewing the applcation with the f_flat_runs feature on, the user should not see the words "experiment" or "trial".

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

netlify · 2024-07-11T19:52:52Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`3275041`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/66a3cf1a2d9cc90008421a27
😎 Deploy Preview	https://deploy-preview-9642--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.

codecov · 2024-07-11T20:46:21Z

Codecov Report

Attention: Patch coverage is 67.60563% with 161 lines in your changes missing coverage. Please review.

Project coverage is 49.91%. Comparing base (7260f04) to head (3275041).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9642      +/-   ##
==========================================
- Coverage   53.44%   49.91%   -3.54%     
==========================================
  Files        1257      934     -323     
  Lines      154350   125265   -29085     
  Branches     3298     3476     +178     
==========================================
- Hits        82494    62521   -19973     
+ Misses      71706    62598    -9108     
+ Partials      150      146       -4

Flag	Coverage Δ
harness	`?`
web	`53.60% <67.60%> (+1.67%)`	⬆️

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

Files	Coverage Δ
webui/react/src/components/CheckpointModal.tsx	`80.26% <100.00%> (+2.44%)`	⬆️
...rc/components/CompareHyperparameters.test.mock.tsx	`100.00% <100.00%> (ø)`
...ui/react/src/components/CompareHyperparameters.tsx	`96.47% <100.00%> (+4.06%)`	⬆️
...bui/react/src/components/ExperimentCreateModal.tsx	`81.58% <100.00%> (+4.11%)`	⬆️
webui/react/src/components/ProjectCard.tsx	`100.00% <100.00%> (ø)`
webui/react/src/components/Searches/columns.ts	`50.40% <100.00%> (ø)`
...ui/react/src/pages/Cluster/ClusterOverallStats.tsx	`89.55% <100.00%> (+0.15%)`	⬆️
webui/react/src/pages/TrialDetails/TrialChart.tsx	`82.73% <100.00%> (+63.46%)`	⬆️
...ebui/react/src/pages/TrialDetails/TrialInfoBox.tsx	`89.25% <100.00%> (+0.08%)`	⬆️
...bui/react/src/components/ExperimentDeleteModal.tsx	`84.31% <80.00%> (+0.98%)`	⬆️
... and 36 more

... and 340 files with indirect coverage changes

thiagodallacqua-hpe

Looking good overall, just a few comments on readability and it would be nice to add a few test cases for all these new content/logic added here

webui/react/src/components/CheckpointModal.tsx

thiagodallacqua-hpe · 2024-07-12T17:57:51Z

webui/react/src/components/ExperimentContinueModal.tsx

+        <Body>
+          {!modalIsInAdvancedMode
+            ? isReactivate
+              ? `Reactivate and continue the current ${entityCopyMap.trial} from the latest checkpoint`
+              : f_flat_runs
+                ? "Start a new run from the current run's latest checkpoint"
+                : "Start a new experiment from the current trial's latest checkpoint"
+            : undefined}
+        </Body>


there's a lot of new code here without coverage as well 😅

also, this is really tricky to read... maybe create a function with if... else to improve the readability

thiagodallacqua-hpe · 2024-07-12T18:00:03Z

webui/react/src/components/ExperimentMoveModal.tsx

+    filters !== undefined
+      ? `Move ${capitalize(entityName)}`
+      : `Move ${capitalize(pluralizer.apply(null, [experimentIds.length, ...(f_flat_runs ? (['search', 'searches'] as const) : (['experiment'] as const))]))}`;


same here, about readability

This one is pretty rough, especially all on one line.

EmilyBonar

Good start, and I know how annoying a job this is!

EmilyBonar · 2024-07-15T20:35:10Z

webui/react/src/pages/FlatRuns/columns.ts

@@ -241,6 +241,7 @@ export const getColumnDefs = ({
    tooltip: () => undefined,
    width: columnWidths.experimentName ?? defaultColumnWidths.experimentName ?? MIN_COLUMN_WIDTH,
  },
+  // TODO: should this change to search?


yeah, probably, for consistency

EmilyBonar · 2024-07-15T20:36:24Z

webui/react/src/pages/ModelDetails.tsx

@@ -431,7 +433,8 @@ const ModelDetails: React.FC = () => {
          <div className={css.noVersions}>
            <p className={css.header}>No Model Versions</p>
            <p className={css.subtext}>
-              Register a checkpoint from an experiment to add it to this model
+              Register a checkpoint from an {f_flat_runs ? 'run' : 'experiment'} to add it to this


nit: 'an run' -> 'a run'

EmilyBonar · 2024-07-15T20:39:42Z

webui/react/src/pages/TrialDetails.tsx

+const ExperimentCopyMap = {
+  experiment: 'experiment',
+  trial: 'trial',
+};
+
+const RunCopyMap = {
+  experiment: 'search',
+  trial: 'run',
+};
+


super nit: since we're standardizing on run/search terminology, maybe we should use those terms to index the maps instead of trial/experiment

EmilyBonar · 2024-07-15T20:40:56Z

webui/react/src/pages/TrialDetails.tsx

@@ -241,7 +259,7 @@ const TrialDetailsComp: React.FC = () => {
              path: paths.workspaceDetails(experiment?.workspaceId ?? 1),
            }
          : {
-              breadcrumbName: 'Uncategorized Experiments',
+              breadcrumbName: `Uncategorized ${f_flat_runs ? 'Experiments' : 'Runs'}`,


nit: might as well use copyMap

EmilyBonar · 2024-07-15T20:42:28Z

webui/react/src/pages/TrialDetails/TrialChart.tsx

@@ -50,14 +52,14 @@ const TrialChart: React.FC<Props> = ({
        setTrialSummary(Loaded(summary[0].metrics));
      } catch (e) {
        handleError(e, {
-          publicMessage: `Failed to load trial summary for trial ${trialId}.`,
-          publicSubject: 'Trial summary fail to load.',
+          publicMessage: `Failed to load ${f_flat_runs ? 'run' : 'trial'} summary for trial ${trialId}.`,


fix: need to replace second instance of 'trial'. could also cut out the first instance and go with "Failed to load summary for [trial/run] ${id}"

EmilyBonar · 2024-07-15T20:57:40Z

webui/react/src/components/ExperimentMoveModal.tsx

+    filters !== undefined
+      ? `Move ${capitalize(entityName)}`
+      : `Move ${capitalize(pluralizer.apply(null, [experimentIds.length, ...(f_flat_runs ? (['search', 'searches'] as const) : (['experiment'] as const))]))}`;


This one is pretty rough, especially all on one line.

EmilyBonar · 2024-07-15T21:00:05Z

webui/react/src/components/ExperimentRetainLogsModal.tsx

          severity: 'Error',
          title: 'Retain Logs Failure',
        });
      } else {
        openToast({
          closeable: true,
-          description: `Failed to retain logs for ${numFailures} experiments out of ${
+          description: `Failed to retain logs for ${numFailures} ${pluralize(2)} out of ${


fix: not sure why that 2 is there, I believe it should be numFailures

EmilyBonar · 2024-07-15T21:01:47Z

webui/react/src/components/ExperimentStopModal.tsx

@@ -24,6 +25,9 @@ interface Props {

 const ExperimentStopModalComponent: React.FC<Props> = ({ experimentId, onClose }: Props) => {
  const [type, setType] = useState<AvalableActions>(ActionType.Cancel);
+  const f_flat_runs = useFeature().isOn('flat_runs');
+
+  const buttonText = f_flat_runs ? 'Stop Search' : BUTTON_TEXT;


nit: this feels like a footgun for a test that relies on BUTTON_TEXT failing

there's nothing importing BUTTON_TEXT from this module, would it make sense to inline?

Yeah, if BUTTON_TEXT isn't being used then definitely

EmilyBonar · 2024-07-15T21:05:33Z

webui/react/src/pages/ExperimentDetails/ExperimentDetailsHeader.tsx

+                    ? 'Create New Run'
+                    : 'Create New Experiment...',


nit: these should both or neither end in ..., unless I'm missing that the Run version doesn't open a modal

EmilyBonar · 2024-07-15T21:09:02Z

webui/react/src/pages/ExperimentDetails/TrialsComparisonModal.tsx

@@ -302,7 +308,7 @@ export const TrialsComparisonTable: React.FC<TableProps> = ({
              {Array.isArray(experiment) && (
                <>
                  <tr>
-                    <th scope="row">Experiment ID</th>
+                    <th scope="row">{f_flat_runs ? 'Search' : 'Experiment'} ID</th>


nit: might as well use entityName

EmilyBonar

Overall looks good

webui/react/src/components/ExperimentStopModal.tsx

webui/react/src/components/HyperparameterSearchModal.test.tsx

webui/react/src/components/CheckpointModal.test.tsx

EmilyBonar · 2024-07-25T20:25:16Z

webui/react/src/components/ExperimentStopModal.test.tsx

  it('submits a valid cancel experiment request', async () => {
    await setup();

-    await user.click(screen.getByRole('button', { name: BUTTON_TEXT }));
+    await user.click(screen.getByRole('button', { name: 'Stop Experiment' }));


nit: use fuzzy matching ({ name: 'Stop', exact: false })

webui/react/src/components/ExperimentStopModal.test.tsx

on vacation

(cherry picked from commit 719f8be)

ashtonG requested a review from a team as a code owner July 11, 2024 19:52

ashtonG requested a review from thiagodallacqua-hpe July 11, 2024 19:52

cla-bot bot added the cla-signed label Jul 11, 2024

ashtonG requested a review from EmilyBonar July 12, 2024 13:37

thiagodallacqua-hpe previously requested changes Jul 12, 2024

View reviewed changes

EmilyBonar suggested changes Jul 15, 2024

View reviewed changes

ashtonG force-pushed the fix/ET-572/run-ux-copy branch 2 times, most recently from a1f69d0 to 4858c3c Compare July 22, 2024 19:33

ashtonG added 12 commits July 25, 2024 11:16

chore: update copy when f_flat_runs is on

6106b46

remove dupe declaration

54cb0cc

fix rebase

ea285c6

add checkpoint modal test

352e539

compare hyperparameters test

9c7e746

blub trial details test

07506f8

mock more

2ac02b2

add mocks

0fb9f92

add copy tests

1abd9f7

add experimentcontinuemodal test

18ae06c

pr feedback

b5fefff

fix affected tests

10fe08a

ashtonG force-pushed the fix/ET-572/run-ux-copy branch from f3ef800 to 10fe08a Compare July 25, 2024 15:40

ashtonG added 2 commits July 25, 2024 12:23

fix typescript issues

e412c26

fix more typescript issues

8c82d83

ashtonG requested review from EmilyBonar and thiagodallacqua-hpe July 25, 2024 16:45

EmilyBonar approved these changes Jul 25, 2024

View reviewed changes

nits

3275041

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

ashtonG merged commit 719f8be into main Jul 26, 2024
81 of 96 checks passed

ashtonG deleted the fix/ET-572/run-ux-copy branch July 26, 2024 17:26

github-actions bot pushed a commit that referenced this pull request Jul 26, 2024

chore: update copy when f_flat_runs is on (#9642)

b6051af

(cherry picked from commit 719f8be)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update copy when f_flat_runs is on #9642

chore: update copy when f_flat_runs is on #9642

ashtonG commented Jul 11, 2024 •

edited by jira bot

Loading

netlify bot commented Jul 11, 2024 •

edited

Loading

codecov bot commented Jul 11, 2024 •

edited

Loading

thiagodallacqua-hpe left a comment

thiagodallacqua-hpe Jul 12, 2024

thiagodallacqua-hpe Jul 12, 2024

thiagodallacqua-hpe Jul 12, 2024

EmilyBonar Jul 15, 2024

EmilyBonar left a comment

EmilyBonar Jul 15, 2024

EmilyBonar Jul 15, 2024

EmilyBonar Jul 15, 2024

EmilyBonar Jul 15, 2024

EmilyBonar Jul 15, 2024

EmilyBonar Jul 15, 2024

EmilyBonar Jul 15, 2024

EmilyBonar Jul 15, 2024

ashtonG Jul 16, 2024

EmilyBonar Jul 23, 2024

EmilyBonar Jul 15, 2024

EmilyBonar Jul 15, 2024

EmilyBonar left a comment

EmilyBonar Jul 25, 2024

chore: update copy when f_flat_runs is on #9642

chore: update copy when f_flat_runs is on #9642

Conversation

ashtonG commented Jul 11, 2024 • edited by jira bot Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented Jul 11, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

codecov bot commented Jul 11, 2024 • edited Loading

Codecov Report

thiagodallacqua-hpe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmilyBonar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmilyBonar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashtonG commented Jul 11, 2024 •

edited by jira bot

Loading

netlify bot commented Jul 11, 2024 •

edited

Loading

codecov bot commented Jul 11, 2024 •

edited

Loading