[UI] Pass namespace to experiment api calls #3252

Bobgy · 2020-03-10T12:23:53Z

/assign @chensun
/area frontend

Pass namespace to all experiment api calls:

listExperiment
createExperiment

@chensun I built UI image that you can try first: gcr.io/gongyuan-pipeline-test/dev/frontend@sha256:f6d3ac5de4994cb3a9ed92e773544e580ec515435502ac5162f69b7f8108504a
I verified the image works, please have a try.

This change is

Bobgy · 2020-03-10T13:18:57Z

/retest

chensun

/lgtm

chensun · 2020-03-11T23:35:53Z

frontend/src/pages/ExperimentList.tsx

@@ -181,6 +182,8 @@ class ExperimentList extends Page<{}, ExperimentListState> {
        request.pageSize,
        request.sortBy,
        request.filter,
+        this.props.namespace ? 'NAMESPACE' : undefined,


Just curious, do we have any constant variable that represents the resource reference type?

There are some problems with current swagger codegen version for frontend. The generated enum cannot pass typechecking here. (tech details...)

But don't worry about the constant here, the function actually accepts only reasonable constants (enforced by typescript typing system): https://github.com/Bobgy/pipelines/blob/10498bf1ad2cd466da59cdb5d58343fa71138ab4/frontend/src/apis/experiment/api.ts#L435-L441

chensun · 2020-03-11T23:41:55Z

/assign @Bobgy

chensun · 2020-03-12T18:31:07Z

@chensun I built UI image that you can try first: gcr.io/gongyuan-pipeline-test/dev/frontend@sha256:f6d3ac5de4994cb3a9ed92e773544e580ec515435502ac5162f69b7f8108504a

Thanks, Yuan. I applied this image in my cluster, and test it with my backend image built from #3243. CreateExperiment, GetExperiment, and ListExperiment all worked as expected except that I saw two issues:

By default, the Kubeflow namespace selector doesn't select any namespace. So at this point, I am able to list all experiments even I don't have access to. And if I click an experiment from a namespace that I don't have access to, I will get an authorization failure as expected.
On the experiment page, when I switch namespace using the namespace selector, the page doesn't auto refresh. So I have to click away from experiment page and then come back to see the refreshed list.

For the first issue, without changing the default behavior of namespace selector, what do you think if we just filter by namespace = '' (empty string) by default? This should be back compatible with the single user mode, as in DB the namespace column in experiment table has the default value of empty string.

Bobgy · 2020-03-13T03:53:30Z

@chensun I built UI image that you can try first: gcr.io/gongyuan-pipeline-test/dev/frontend@sha256:f6d3ac5de4994cb3a9ed92e773544e580ec515435502ac5162f69b7f8108504a

Thanks, Yuan. I applied this image in my cluster, and test it with my backend image built from #3243. CreateExperiment, GetExperiment, and ListExperiment all worked as expected except that I saw two issues:

By default, the Kubeflow namespace selector doesn't select any namespace. So at this point, I am able to list all experiments even I don't have access to. And if I click an experiment from a namespace that I don't have access to, I will get an authorization failure as expected.

When namespace is not selected, should backend api reject the request instead? I can add UI to inform user they must select a namespace. (of course, these changes should be under feature flag for multi user mode)

On the experiment page, when I switch namespace using the namespace selector, the page doesn't auto refresh. So I have to click away from experiment page and then come back to see the refreshed list.

Yep, that's a good point. Can you share your backend image with me? I will keep adding fixes in following PRs. These new changes break existing UI assumptions, so I need to add some fixes.

For the first issue, without changing the default behavior of namespace selector, what do you think if we just filter by namespace = '' (empty string) by default? This should be back compatible with the single user mode, as in DB the namespace column in experiment table has the default value of empty string.

I see, sounds like a good idea. Just clarify, you will add the default behavior in backend, right? When frontend doesn't specify namespace resource reference, backend will list experiment by namespace=''?

chensun · 2020-03-13T07:04:30Z

@chensun I built UI image that you can try first: gcr.io/gongyuan-pipeline-test/dev/frontend@sha256:f6d3ac5de4994cb3a9ed92e773544e580ec515435502ac5162f69b7f8108504a

Thanks, Yuan. I applied this image in my cluster, and test it with my backend image built from #3243. CreateExperiment, GetExperiment, and ListExperiment all worked as expected except that I saw two issues:

By default, the Kubeflow namespace selector doesn't select any namespace. So at this point, I am able to list all experiments even I don't have access to. And if I click an experiment from a namespace that I don't have access to, I will get an authorization failure as expected.

When namespace is not selected, should backend api reject the request instead? I can add UI to inform user they must select a namespace. (of course, these changes should be under feature flag for multi user mode)

On the experiment page, when I switch namespace using the namespace selector, the page doesn't auto refresh. So I have to click away from experiment page and then come back to see the refreshed list.

Yep, that's a good point. Can you share your backend image with me? I will keep adding fixes in following PRs. These new changes break existing UI assumptions, so I need to add some fixes.

Here's the backend image (thanks for reminding me, I should have this in my PR description as well): gcr.io/chesu-dev/api-server@sha256:4ad96dc8862bffb74a73fd305b17ca33fa8cef6f3eaf0dd2f7f1094ed8365c78

For the first issue, without changing the default behavior of namespace selector, what do you think if we just filter by namespace = '' (empty string) by default? This should be back compatible with the single user mode, as in DB the namespace column in experiment table has the default value of empty string.

I see, sounds like a good idea. Just clarify, you will add the default behavior in backend, right? When frontend doesn't specify namespace resource reference, backend will list experiment by namespace=''?

I was thinking having frontend send this empty namespace filter in the request URL, such as "resource_reference_key.type=NAMESPACE&resource_reference_key.id=". Is this doable?
I'd imagine that on the frontend side, there's a global/context variable that stores what namespace is selected, and by default when none is selected, this variable has the value of an empty string. If so, we can just pass the reference key filter always regardless the namespace value, which feels more nature to me. And in the future, if the namespace selector changed to have a default selection, no code need to be changed.
If this is done on the backend side, then we would have a logic like if no filter, then add filter with empty value, which feels a bit hacky to me. WDYT?

Bobgy · 2020-03-13T07:25:46Z

Thanks for the image.

Regarding:

I was thinking having frontend send this empty namespace filter in the request URL, such as "resource_reference_key.type=NAMESPACE&resource_reference_key.id=". Is this doable?
I'd imagine that on the frontend side, there's a global/context variable that stores what namespace is selected, and by default when none is selected, this variable has the value of an empty string. If so, we can just pass the reference key filter always regardless the namespace value, which feels more nature to me. And in the future, if the namespace selector changed to have a default selection, no code need to be changed.
If this is done on the backend side, then we would have a logic like if no filter, then add filter with empty value, which feels a bit hacky to me. WDYT?

This is hacky logic no matter where we put it, my original reasoning to put it in backend was to delay the hacky logic to the last step we can make a decision.

If frontend sends empty namespace instead, backend will not be able to tell whether the client selected a namespace and it's empty (might be an error), or is it because the client didn't select a namespace.

I agree the difference here is subtle, so it may not be a big deal.

Another (stronger) point is that, system architecture for KFP is: "multiple clients and a single backend server", You should consider HTTP clients, python clients and UI clients as all valid usages. If we add the extra logic in UI, then we also need to duplicate it in other clients. e.g. kfp.Client().list_experiments() should also return experiments in "" namespace, or get rejected because it didn't choose namespace.

Bobgy · 2020-03-13T08:24:10Z

/retest

chensun · 2020-03-13T21:21:55Z

This is hacky logic no matter where we put it, my original reasoning to put it in backend was to delay the hacky logic to the last step we can make a decision.

If frontend sends empty namespace instead, backend will not be able to tell whether the client selected a namespace and it's empty (might be an error), or is it because the client didn't select a namespace.

I agree the difference here is subtle, so it may not be a big deal.

Another (stronger) point is that, system architecture for KFP is: "multiple clients and a single backend server", You should consider HTTP clients, python clients and UI clients as all valid usages. If we add the extra logic in UI, then we also need to duplicate it in other clients. e.g. kfp.Client().list_experiments() should also return experiments in "" namespace, or get rejected because it didn't choose namespace.

Regarding the point about multiple clients, I put up the SDK changes here: #3272. As you can see, I simply pass whatever value of namespace to the request filter. It actually requires less logic on the client sides, rather than "extra logic". That's why I thought it's not hacky. Do you think the same applies to web UI? In this way, backend just do whatever the clients request. And I feel like it's simpler logic on both ends.
In comparison, if we default to empty namespace on backend, it feels like backend is altering the request: if request contains an explicit filter, backend follows the filter; if request doesn't contain a filter, backend adds a 'special' filter.
Just my two cents, while I agree the difference isn't that significant. So at the end, I would be fine with either approach.

Bobgy · 2020-03-16T06:21:50Z

/retest

Bobgy · 2020-03-16T06:59:54Z

Thanks for the explanation, that starts to make more sense to me. Just want to sync about our expected CUJs from users.

multi user mode

User should always specify a namespace.

If namespace is empty, backend should probably return a list with experiments before enabling multi user mode (or empty list if the cluster was multi user mode from the beginning). (I agree this point is arguable, we can also just return an error message saying the user must choose a namespace.)
If namespace not provided, backend should reject the request with an error message. (Error message is better than empty list because it clearly tells the user what should be next step.)

single user mode

No matter what, the backend should return a list of all experiments with namespace "".
(probably, we should give some error messages if a non empty namespace is specified)

How can we achieve the CUJ?

We must be able to tell whether a user intentionally chose to query for empty namespace, or simply didn't choose a namespace in multi user mode.

Another point: backward compatibility

I'm not sure how many, but a small portion of our users do actually use backend api endpoint directly. (I saw once during a discussion in kubeflow slack, someone mentioned they just use kfp backend api endpoints directly, instead of the python sdk.) Having to add a resource reference for experiment in single user mode is a breaking change.

UPDATE: I found it, you can take a look at this thread: https://kubeflow.slack.com/archives/CE10KS9M4/p1583676641132500

If we all agree both choices only differ in subtle ways, maybe it's better to do fewer breaking changes. What do you think?

Bobgy · 2020-03-16T16:21:49Z

Hey, actually I just got the idea that, to achieve above CUJ, we don't need to alternate request to add default empty namespace in backend.

The only required change is to reject queries without namespace in multi user mode.

chensun · 2020-03-16T18:23:08Z

Hey, actually I just got the idea that, to achieve above CUJ, we don't need to alternate request to add default empty namespace in backend.

The only required change is to reject queries without namespace in multi user mode.

The CUJ looks good to me. I agree that an explicit error is better than an empty list.
I will make the change to reject empty namespace for all scenarios in multi-user mode.

For single-user mode though, in order to return experiments with empty namespace. Either clients has to pass in filter with empty namespace, or backend will have to alter the request. Given the slack thread you mentioned above, I think altering request in the backend is necessary.

/lgtm
/assign @IronPan

Bobgy · 2020-03-17T03:25:01Z

@chensun Sounds good! Thanks for the thorough discussion!

/approve

k8s-ci-robot · 2020-03-17T03:25:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy

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:

~~frontend/OWNERS~~ [Bobgy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Bobgy · 2020-03-17T04:23:15Z

/retest

Bobgy · 2020-03-17T05:54:31Z

/retest

* Regenerate frontend api client * [UI] Add namespace resource reference to experiment api call * Pass namespace to experiment list api call * Add namespace to NewRun page api calls

Bobgy added 4 commits March 10, 2020 17:59

Regenerate frontend api client

902bdf8

[UI] Add namespace resource reference to experiment api call

de7fb60

Pass namespace to experiment list api call

d84658e

Add namespace to NewRun page api calls

10498bf

k8s-ci-robot assigned chensun Mar 10, 2020

k8s-ci-robot added area/frontend size/L labels Mar 10, 2020

k8s-ci-robot requested review from IronPan and neuromage March 10, 2020 12:24

chensun reviewed Mar 11, 2020

View reviewed changes

k8s-ci-robot added the lgtm label Mar 11, 2020

k8s-ci-robot assigned Bobgy Mar 11, 2020

k8s-ci-robot assigned IronPan Mar 16, 2020

k8s-ci-robot added the approved label Mar 17, 2020

k8s-ci-robot merged commit 0dca4fe into kubeflow:master Mar 17, 2020

Bobgy deleted the fe-add-ns-to-experiment branch March 17, 2020 08:00

Bobgy mentioned this pull request Mar 17, 2020

[Multi User] UI should react when user changes namespace #3296

Closed

snyk-bot mentioned this pull request May 26, 2021

[Snyk] Upgrade axios from 0.19.2 to 0.21.1 MaxKelsen/pipelines#10

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UI] Pass namespace to experiment api calls #3252

[UI] Pass namespace to experiment api calls #3252

Bobgy commented Mar 10, 2020 •

edited

Loading

Bobgy commented Mar 10, 2020

chensun left a comment

chensun Mar 11, 2020

Bobgy Mar 12, 2020

chensun commented Mar 11, 2020

chensun commented Mar 12, 2020

Bobgy commented Mar 13, 2020

chensun commented Mar 13, 2020

Bobgy commented Mar 13, 2020

Bobgy commented Mar 13, 2020

chensun commented Mar 13, 2020

Bobgy commented Mar 16, 2020

Bobgy commented Mar 16, 2020

Bobgy commented Mar 16, 2020

chensun commented Mar 16, 2020

Bobgy commented Mar 17, 2020

k8s-ci-robot commented Mar 17, 2020

Bobgy commented Mar 17, 2020

Bobgy commented Mar 17, 2020

[UI] Pass namespace to experiment api calls #3252

[UI] Pass namespace to experiment api calls #3252

Conversation

Bobgy commented Mar 10, 2020 • edited Loading

Bobgy commented Mar 10, 2020

chensun left a comment

Choose a reason for hiding this comment

chensun Mar 11, 2020

Choose a reason for hiding this comment

Bobgy Mar 12, 2020

Choose a reason for hiding this comment

chensun commented Mar 11, 2020

chensun commented Mar 12, 2020

Bobgy commented Mar 13, 2020

chensun commented Mar 13, 2020

Bobgy commented Mar 13, 2020

Bobgy commented Mar 13, 2020

chensun commented Mar 13, 2020

Bobgy commented Mar 16, 2020

Bobgy commented Mar 16, 2020

multi user mode

single user mode

How can we achieve the CUJ?

Another point: backward compatibility

Bobgy commented Mar 16, 2020

chensun commented Mar 16, 2020

Bobgy commented Mar 17, 2020

k8s-ci-robot commented Mar 17, 2020

Bobgy commented Mar 17, 2020

Bobgy commented Mar 17, 2020

Bobgy commented Mar 10, 2020 •

edited

Loading