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

chore: migrate experiment list api [DET-3696, DET-3697, DET-3698] #1228

Merged
merged 15 commits into from
Sep 2, 2020

Conversation

hkang1
Copy link
Contributor

@hkang1 hkang1 commented Sep 2, 2020

Description

We hit a wall with large experiment lists with the old API earlier than anticipated. Migrate over to the new API for experiment list to introduce pagination to break up the massive experiment list. UI should remain the same, except the pagination interactions triggers additional paginated calls to the API.

Screen Shot 2020-09-02 at 9 50 57 AM

Test Plan

Commentary (optional)

Checklist

  • 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.

@cla-bot cla-bot bot added the cla-signed label Sep 2, 2020
@hkang1 hkang1 changed the title chore: migrate experiment list api [DET-3698] chore: migrate experiment list api [DET-3696, DET-3698] Sep 2, 2020
@hkang1 hkang1 changed the title chore: migrate experiment list api [DET-3696, DET-3698] chore: migrate experiment list api [DET-3696, DET-3697, DET-3698] Sep 2, 2020
@hkang1 hkang1 marked this pull request as ready for review September 2, 2020 15:50
@hkang1 hkang1 requested a review from hamidzr September 2, 2020 15:55
Copy link
Contributor

@justin-determined-ai justin-determined-ai left a comment

Choose a reason for hiding this comment

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

Nice work putting this together so quickly! I think it looks good, with a few questions I was unsure of.

webui/react/src/components/Table.stories.tsx Show resolved Hide resolved
webui/react/src/components/Table.tsx Show resolved Hide resolved
webui/react/src/components/TaskCard.tsx Show resolved Hide resolved
webui/react/src/pages/Dashboard.tsx Show resolved Hide resolved
webui/react/src/types.ts Show resolved Hide resolved
Copy link
Member

@hamidzr hamidzr left a comment

Choose a reason for hiding this comment

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

Thanks for getting this out quickly! I added comments going through the code. I still need to do a visual check. WIP

webui/react/src/services/api.ts Outdated Show resolved Hide resolved
webui/react/src/services/api.ts Outdated Show resolved Hide resolved
webui/react/src/services/api.ts Outdated Show resolved Hide resolved
@@ -260,6 +262,47 @@ export const jsonToTrialDetails = (data: unknown): TrialDetails => {
};
};

const experimentStateMap = {
Copy link
Member

Choose a reason for hiding this comment

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

for later: we'll have a bunch of these I think we could start mapping them in a separate file.

Copy link
Member

Choose a reason for hiding this comment

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

and maybe the mapping functions below for state as they're used elsewhere

webui/react/src/pages/Dashboard.tsx Outdated Show resolved Hide resolved
webui/react/src/pages/Dashboard.tsx Show resolved Hide resolved
webui/react/src/pages/ExperimentList.tsx Outdated Show resolved Hide resolved
webui/react/src/components/TaskCard.tsx Outdated Show resolved Hide resolved
@hamidzr
Copy link
Member

hamidzr commented Sep 2, 2020

most of the comments are on type and code structure so I think we could leave a big chunk of it for later if we need to get pagination in quickly. visually testing it now

@hamidzr
Copy link
Member

hamidzr commented Sep 2, 2020

comparing the response sizes: (limit 10) (total 25)
before:
Screen Capture_select-area_20200902132942
after:
Screen Capture_select-area_20200902132650

@hamidzr hamidzr assigned hkang1 and unassigned hamidzr Sep 2, 2020
@hkang1 hkang1 removed their assignment Sep 2, 2020
@hkang1 hkang1 force-pushed the 3698-migrate-experiment-list-api branch from dee760e to bbd3c50 Compare September 2, 2020 21:51
Copy link
Member

@hamidzr hamidzr 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/Table.tsx Outdated Show resolved Hide resolved
webui/react/src/services/api.ts Outdated Show resolved Hide resolved
@hkang1 hkang1 force-pushed the 3698-migrate-experiment-list-api branch from bbd3c50 to a3cf629 Compare September 2, 2020 22:21
@hamidzr hamidzr assigned hkang1 and unassigned hamidzr Sep 2, 2020
@hkang1 hkang1 merged commit 98036f0 into determined-ai:master Sep 2, 2020
@hkang1 hkang1 deleted the 3698-migrate-experiment-list-api branch September 2, 2020 22:43
justin-determined-ai pushed a commit to justin-determined-ai/determined that referenced this pull request Sep 2, 2020
@dannysauer dannysauer added this to the 0.13.4 milestone Feb 6, 2024
eecsliu pushed a commit to determined-ai/determined-release-testing that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants