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

feat: pin experiments #4925

Merged

Conversation

keita-determined
Copy link
Contributor

@keita-determined keita-determined commented Aug 31, 2022

Description

DET-8135

Test Plan

  • Check if experiment item can be pinned or unpinned
  • Check if experiment items stick to the top of table
  • Check if there are pinned items on each pagination page on top
  • Check if pagination works well
  • Check if pinned experiment should be removed after Move Action
  • Check if pin experiments up to 5 in each project

Commentary (optional)

  • Netlify doesn't work as expected cuz i changed backend

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.
  • If modifying /webui/react/src/shared/ verify make -C webui/react test-shared passes.

@cla-bot cla-bot bot added the cla-signed label Aug 31, 2022
@netlify
Copy link

netlify bot commented Aug 31, 2022

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 65c2a7b
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/631fc8a4cf9672000981c459
😎 Deploy Preview https://deploy-preview-4925--determined-ui.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@netlify
Copy link

netlify bot commented Aug 31, 2022

Deploy Preview for storybook-det ready!

Name Link
🔨 Latest commit 65c2a7b
🔍 Latest deploy log https://app.netlify.com/sites/storybook-det/deploys/631fc8a4c0c5660008115c0f
😎 Deploy Preview https://deploy-preview-4925--storybook-det.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@keita-determined keita-determined force-pushed the feature/pin-experiment branch 3 times, most recently from 8ef6683 to 0bf305e Compare September 1, 2022 19:54
@keita-determined keita-determined mentioned this pull request Sep 6, 2022
5 tasks
@keita-determined keita-determined marked this pull request as ready for review September 8, 2022 01:17
@keita-determined keita-determined marked this pull request as draft September 8, 2022 01:17
@keita-determined keita-determined force-pushed the feature/pin-experiment branch 2 times, most recently from 6da648d to 01cba6d Compare September 8, 2022 06:46
@@ -43,6 +43,23 @@ message GetExperimentResponse {

// Get a list of experiments.
message GetExperimentsRequest {
// filtering by experiment ids
message ExperimentFilter {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont have good naming sense. Any better name?

Copy link
Contributor

@EmilyBonar EmilyBonar left a comment

Choose a reason for hiding this comment

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

Looks cool so far, but needs some more polish and testing when it comes to table and pagination issues

webui/react/src/utils/experiment.ts Outdated Show resolved Hide resolved
webui/react/src/components/ExperimentActionDropdown.tsx Outdated Show resolved Hide resolved
webui/react/src/components/ExperimentActionDropdown.tsx Outdated Show resolved Hide resolved
webui/react/src/components/ExperimentActionDropdown.tsx Outdated Show resolved Hide resolved
webui/react/src/components/ExperimentActionDropdown.tsx Outdated Show resolved Hide resolved
webui/react/src/pages/ProjectDetails.settings.ts Outdated Show resolved Hide resolved
webui/react/src/pages/ProjectDetails.tsx Outdated Show resolved Hide resolved
webui/react/src/pages/ProjectDetails.tsx Outdated Show resolved Hide resolved
webui/react/src/pages/ProjectDetails.tsx Outdated Show resolved Hide resolved
webui/react/src/pages/ProjectDetails.tsx Outdated Show resolved Hide resolved
@@ -43,6 +43,23 @@ message GetExperimentResponse {

// Get a list of experiments.
message GetExperimentsRequest {
// filtering by experiment ids
message ExperimentFilter {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a question about REST API design here.
this ends up generating URLs like this: /api/v1/experiments?sortBy=SORT_BY_ID&orderBy=ORDER_BY_ASC&offset=0&limit=6&archived=false&projectId=104&experimentFilter.restriction=RESTRICTION_EXCLUDE&experimentFilter.experimentIds=125&experimentFilter.experimentIds=126&experimentFilter.experimentIds=34&experimentFilter.experimentIds=105

it's a bit verbose. some clients / systems sometimes have an upper limit on the URL length (at ~2k characters IIRC). this URL format spends 30+len(experiment_id) characters per experiment, so it'd hit a limit at 66 experiments which doesn't sound unrealistic to me.

if you were building this REST API without the luggage of grpc-gateway and its enums, how would you want this API URL to look like?

one possible suggestion here is to do something like this: /api/v1/experiments?sortBy=SORT_BY_ID&orderBy=ORDER_BY_ASC&offset=0&limit=6&archived=false&projectId=104&experimentIds__in=125,126,34,105, i.e. "lookup field" and "lookup operation" in a single query parameter, separated by double underscore.

  • for "exclusion" it could look like experimentIds__notin=125,126,34,105
  • this provides a basis for other operations like gte for greater or equal, lt for less than, etc. allowing a generic search operations such as ?experimentIds__lte=100&experimentIds__gt=50 for looking up 50 < experiment id <= 100, as an example.

we currently do not have examples of this approach implemented in the code but it may be worth investing in for the sake of good API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally wanted to have the one you suggested ids=1,2,3.
Let me take a look at how REST API is generated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the documentation ,

All other fields are passed via the URL query parameters, and the parameter name is the field path in the request message. A repeated field can be represented as multiple query parameters under the same name.

We might not able to change the style of param query in generated REST API.
I'll search more but if we cant change it all we can do with repeated type is to use shorter variable name or use post as get but its not ideal.

Copy link
Contributor

Choose a reason for hiding this comment

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

push comes to shove, it can be a string in proto that is split by , on server side.

master/internal/api_experiment.go Outdated Show resolved Hide resolved
@keita-determined
Copy link
Contributor Author

@EmilyBonar

  • Add limit up to 5 pinned items <- that fixes pagination issue
  • Replaced Pin and Unpin with SwitchPin
  • Change data structure of pinned (KV type)
  • Remove pin icon. Instead, add border in the end of pinned items
  • Support Move action

Copy link
Contributor

@EmilyBonar EmilyBonar left a comment

Choose a reason for hiding this comment

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

Fix these last couple things and you should be good

@ioga ioga mentioned this pull request Sep 9, 2022
3 tasks
@keita-determined
Copy link
Contributor Author

Wait for the backend change to archive more complex filtering
Ref: https://determined-ai.slack.com/archives/CKQ3EV0G6/p1662755531081139

@@ -122,7 +107,7 @@ message GetExperimentsRequest {
// projects.
int32 project_id = 12;
// filtering by experiment ids
ExperimentFilter experiment_filter = 13;
determined.common.v1.Int32FieldFilter experiment_filter = 13;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe rename experiment_id_filter

@keita-determined keita-determined enabled auto-merge (squash) September 13, 2022 00:20
@keita-determined keita-determined merged commit 311cb11 into determined-ai:master Sep 13, 2022
@keita-determined keita-determined deleted the feature/pin-experiment branch September 13, 2022 00:29
@dannysauer dannysauer modified the milestones: 0.0.102, 0.19.4 Feb 6, 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