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.

Sign up for GitHub

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

Jump to bottom

fix(sdk): Respect namespace if set on client #7056

Merged
merged 9 commits into from
Jan 11, 2022
Merged

fix(sdk): Respect namespace if set on client #7056

merged 9 commits into from
Jan 11, 2022

Conversation

casassg
Copy link
Contributor

@casassg casassg commented Dec 13, 2021

Description of your changes:

Currently kfp.Client ignores if namespace is set on constructor. This leads to having to shortly after having to invoke set_user_namespace, which makes it non-obvious.

Instead here, we add set_user_namespace on constructor, which allows us to avoid having to do 2 calls.

@google-oss-prow
Copy link

Hi @casassg. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

chensun
chensun reviewed Dec 16, 2021
View reviewed changes
Copy link
Member

@chensun chensun left a comment

Choose a reason for hiding this comment

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

/ok-to-test

sdk/python/kfp/_client.py Outdated Show resolved Hide resolved
chensun
chensun reviewed Dec 16, 2021
View reviewed changes
Copy link
Member

@chensun chensun left a comment

Choose a reason for hiding this comment

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

/hold

sdk/python/kfp/_client.py Outdated
@@ -188,8 +194,7 @@ def __init__(self,
api_client)
self._healthz_api = kfp_server_api.api.healthz_service_api.HealthzServiceApi(
Copy link
Member

Choose a reason for hiding this comment

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

Actually, self.get_kfp_healthz() relies on self._healthz_api being initialized first. If you move self.get_kfp_healthz() up, it will never succeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True that. Made logic simpler.

I basically set the namespace if user has explicitly called out namespace. I only ovewrite current instance though to avoid having side effects and changing the default which is written in the file.

@google-oss-prow google-oss-prow bot added size/S and removed size/XS labels Dec 16, 2021
@google-oss-prow google-oss-prow bot added size/XS and removed size/S labels Dec 16, 2021
@casassg
Copy link
Contributor Author

casassg commented Dec 22, 2021

@chensun guessing you are on vacation, just wanted to gently message here :D

Happy holidays!

@chensun
Copy link
Member

chensun commented Jan 6, 2022

/lgtm
/unhold

@casassg Can you please add a release note for this change?
Also, if you can help cherry pick the change back to sdk/release-1.8 branch, that would be great.

Thanks!

@google-oss-prow google-oss-prow bot added the lgtm label Jan 6, 2022
@google-oss-prow google-oss-prow bot removed the lgtm label Jan 10, 2022
@casassg
Copy link
Contributor Author

casassg commented Jan 10, 2022

@chensun I can create the cherry-pick as it gets merged, since commit will be squashed then

chensun
chensun reviewed Jan 10, 2022
View reviewed changes
sdk/RELEASE.md Outdated
@@ -9,6 +9,7 @@
* Add ImporterSpec v2 [\#6917](https://github.com/kubeflow/pipelines/pull/6917)
* Add add set_env_variable for Pipeline task [\#6919](https://github.com/kubeflow/pipelines/pull/6919)
* Add metadata field for importer [\#7112](https://github.com/kubeflow/pipelines/pull/7112)
* kfp.Client uses namespace from initialization if set for the instance context.
Copy link
Member

Choose a reason for hiding this comment

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

nit: Add a link to the PR like the rest.

@chensun
Copy link
Member

chensun commented Jan 11, 2022

/lgtm
/approve

Thanks!

@google-oss-prow google-oss-prow bot added the lgtm label Jan 11, 2022
@google-oss-prow
Copy link

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit fc2d05f into kubeflow:master Jan 11, 2022
chensun pushed a commit to chensun/pipelines that referenced this pull request Jan 21, 2022
@casassg @chensun
fix(sdk): Respect namespace if set on client (kubeflow#7056)
7c2830e 
* respect namespace if set on client

* make sure is_multi_user

* overwrite directly context_setting

* revert non-related changes

* revert overwrite_namespace

* expand text

* add release notes

* add PR link
@casassg casassg deleted the gcasassaez/fix-namespace-client branch January 22, 2022 03:38
chensun added a commit that referenced this pull request Jan 24, 2022
@chensun @casassg @SebBlin
chore(sdk1.8): cherry pick #6866, #7056, #7141, #7143, #7168, #7172, #…
7b2ca38 
…7182 into sdk/release-1.8 branch. (#7200)

* test: fix tfx presubmit test. (#7143)

* fix(sdk): Respect namespace if set on client (#7056)

* respect namespace if set on client

* make sure is_multi_user

* overwrite directly context_setting

* revert non-related changes

* revert overwrite_namespace

* expand text

* add release notes

* add PR link

* fix(sdk): Support per workflow TTL (ttl_seconds_after_finished) with new format of Argo workflow manifest (#7141)

* Support per workflow TTL (ttl_seconds_after_finished) with new format of Argo workflow manifest

* Update test for TTL

* Declare fix in release note of SDK

* update syntaxe

* Update RELEASE.md

Add Braking change due to incompatibility with  KFP pre 1.7 due to Argo 2.X

* chore(sdk): Remove redundant check in set_gpu_limit (#6866)

* remove redundant check in set_gpu_limit

* release note

* fix(sdk): Fix create_runtime_artifact not covering all types. (#7168)

* Fix create_runtime_artifact not covering all types.

* release note

* fix(sdk): bumps up absl version to 1.x for compatibility with TF 2.8+. Fixes #7171 (#7172)

* Bumps up absl version to 1.x for compatibility with TF 2.8+.

* Adds a release note for absl version.

* chore(sdk): Update SDK release note for the upcoming 1.8.11 release (#7182)

Co-authored-by: Gerard Casas Saez <gcasassaez@twitter.com>
Co-authored-by: Seb <sebastien.blin@gmail.com>
Co-authored-by: Jiyong Jung <jiyongjung@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chensun chensun chensun left review comments

@ji-yaqi ji-yaqi Awaiting requested review from ji-yaqi

Assignees

@chensun chensun

Labels
approved lgtm ok-to-test size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@casassg @chensun