-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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(frontend/backend): Allow the ability to sort experiments by last run creation. Fixes #10884 #11163
Conversation
Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com>
…0884 Signed-off-by: Elay Aharoni (EXT-Nokia) <elay.aharoni.ext@nokia.com>
Hi @ElayAharoni. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
…Timestamp follup up to bf77909. Rename UpdateLastRun -> SetLastRunTimestamp also tweak a related log message Signed-off-by: Greg Sheremeta <gshereme@redhat.com>
/ok-to-test |
…Timestamp follup up to bf77909. Rename UpdateLastRun -> SetLastRunTimestamp also tweak a related log message Signed-off-by: Greg Sheremeta <gshereme@redhat.com>
…ments-by-last-run-creation-#10884' into Allow-the-ability-to-sort-Experiments-by-last-run-creation-kubeflow#10884
Name string `gorm:"column:Name; not null; unique_index:idx_name_namespace;"` | ||
Description string `gorm:"column:Description; not null;"` | ||
CreatedAtInSec int64 `gorm:"column:CreatedAtInSec; not null;"` | ||
LastRunCreatedAtInSec int64 `gorm:"column:LastRunCreatedAtInSec; not null;"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chensun fyi, this change adds a new column to Experiment table to allow for tracking for the last created run. This is required because list sorting performs sql sort operations on columns under the hood, and currently there is no way to do this with the existing schema.
Also note here: https://github.com/kubeflow/pipelines/pull/11163/files#diff-62d02d47cfac0eb11fdad7cf760bac31eb64f26e05aec490934c05bfd7949c09R569 which makes an additional db call during run creation calls.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding new columns should be fine. Thanks for the callout.
cc @chensun , please see comments above |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chensun The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
… run creation. Fixes kubeflow#10884 (kubeflow#11163) * UPSTREAM: <carry>: add last_run_creation Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com> * Allow-the-ability-to-sort-Experiments-by-last-run-creation-kubeflow#10884 Signed-off-by: Elay Aharoni (EXT-Nokia) <elay.aharoni.ext@nokia.com> * UPSTREAM: <carry>: chore(backend): Rename UpdateLastRun -> SetLastRunTimestamp follup up to bf77909. Rename UpdateLastRun -> SetLastRunTimestamp also tweak a related log message Signed-off-by: Greg Sheremeta <gshereme@redhat.com> * UPSTREAM: <carry>: chore(backend): Rename UpdateLastRun -> SetLastRunTimestamp follup up to bf77909. Rename UpdateLastRun -> SetLastRunTimestamp also tweak a related log message Signed-off-by: Greg Sheremeta <gshereme@redhat.com> --------- Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com> Signed-off-by: Elay Aharoni (EXT-Nokia) <elay.aharoni.ext@nokia.com> Signed-off-by: Greg Sheremeta <gshereme@redhat.com> Co-authored-by: Humair Khan <HumairAK@users.noreply.github.com> Co-authored-by: Elay Aharoni (EXT-Nokia) <elay.aharoni.ext@nokia.com> Co-authored-by: Greg Sheremeta <gshereme@redhat.com>
Fixes #10884.
I cherry picked @HumairAK commit for the BE side.
and did the necessary changes to the FE side.
also built the image and tested the behaviour, it works as requested. (attached video of the new behaviour).
https://www.loom.com/share/4ce09c4c87464285a744cff42d42aa2f?sid=e65c93d4-3116-4f05-8066-66fd0115de63