feat(backend): sort by run metrics - step 3. Part of #3591 #4251
Conversation
… the archive page
…page and the archive page" This reverts commit 5b67273.
…ed at, etc. The lattre are direct field in listable object, the former is an element in an arrary-typed field in listable object. In other words, the latter are columns in table, the former is not.
|
This change is |
backend/src/apiserver/list/list.go
Outdated
| // The listable model this token is applied to. Not used in json marshal/unmarshal. | ||
| Model Listable `json:"-"` | ||
| // ModelType and the ModelMessage are helper fields to unmarshal data correctly to | ||
| // the underlying listable model, and this underlying listable model will be stored | ||
| // in the above Model field. Those two fields are only used in token's marshal and | ||
| // unmarshal methods. | ||
| ModelType string | ||
| ModelMessage json.RawMessage |
There was a problem hiding this comment.
nit: Can you comment example values of these fields?
That helps understanding these fields.
My initial confusion when reading this: Is it an ML model or a data model?
There was a problem hiding this comment.
Actually this is not about the ML terminology, but about a terminology used in backend implementation . We actually can get clues from the existing backend implementation. If we take a look at our code, there is a folder called backend/src/apiserver/model. In that folder, we have structs job, run, experiment, etc. In other words, the run, job, experiment, etc. are considered a type of "model". (I am not sure about the history here though). Moreover, all those models (aka, job, run, experiment, etc.) implement the "listable" interface. In other words, the set of jobs, (or runs, experiments) is listable.
So back to this struct token. Token is applied to a listable (aka a set of runs, jobs, experiments etc.). So, in the token, we can add this model field to indicate which model (aka, run, job, experiment) this token applies to.
There was a problem hiding this comment.
So I add some explanation to this field, pointing to the folder. Hope that would help.
|
In my manual test in a real deployment, it turns out the "Model Listable" field has too large a data size when listing runs (other types are fine), since runs contain the complete manifest. Therefore, I replaced "Model Listable" with only relevant fields in Listable type. |
relevant fields taken out of it. In the future, if more fields in Listable object become relevant, manually add it to token
|
/test kubeflow-pipeline-e2e-test |
Bobgy
changed the title
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jingzhang36 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 |
…ubeflow#4251) * enable pagination when expanding experiment in both the home page and the archive page * Revert "enable pagination when expanding experiment in both the home page and the archive page" This reverts commit 5b67273. * sorting by run metrics is different from sorting by name, uuid, created at, etc. The lattre are direct field in listable object, the former is an element in an arrary-typed field in listable object. In other words, the latter are columns in table, the former is not. * unit test: add sorting on metrics with both asc and desc order * GetFieldValue in all models * fix unit test * whether to test in list_test. It's hacky when check mode == 'run' * move model specific code to model; prevent model package depends on list package; let list package depends on modelpackage; marshal/unmarshal listable interface; include listable interface in token. * some assumption on token's Model field * fix the regular field checking logic * add comment to help devs to use the new field * add a validation check * Listable object can be too large to be in token. So replace it with only relevant fields taken out of it. In the future, if more fields in Listable object become relevant, manually add it to token * matches func update
…ubeflow#4251) * enable pagination when expanding experiment in both the home page and the archive page * Revert "enable pagination when expanding experiment in both the home page and the archive page" This reverts commit 5b67273. * sorting by run metrics is different from sorting by name, uuid, created at, etc. The lattre are direct field in listable object, the former is an element in an arrary-typed field in listable object. In other words, the latter are columns in table, the former is not. * unit test: add sorting on metrics with both asc and desc order * GetFieldValue in all models * fix unit test * whether to test in list_test. It's hacky when check mode == 'run' * move model specific code to model; prevent model package depends on list package; let list package depends on modelpackage; marshal/unmarshal listable interface; include listable interface in token. * some assumption on token's Model field * fix the regular field checking logic * add comment to help devs to use the new field * add a validation check * Listable object can be too large to be in token. So replace it with only relevant fields taken out of it. In the future, if more fields in Listable object become relevant, manually add it to token * matches func update
Description of your changes:
Part of #3591
Some refactoring to the existing listable interface and its implementors (i.e., various models of run, job, experiment, pipeline and pipeline version)
Checklist:
The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.
PR titles examples:
fix(frontend): fixes empty page. Fixes #1234Use
fixto indicate that this PR fixes a bug.feat(backend): configurable service account. Fixes #1234, fixes #1235Use
featto indicate that this PR adds a new feature.chore: set up changelog generation toolsUse
choreto indicate that this PR makes some changes that users don't need to know.test: fix CI failure. Part of #1234Use
part ofto indicate that a PR is working on an issue, but shouldn't close the issue when merged.Do you want this pull request (PR) cherry-picked into the current release branch?
If yes, use one of the following options:
cherrypick-approvedlabel to this PR. The release manager adds this PR to the release branch in a batch update.