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

[Doc] Change sample/component/sdk documentation to not use use_gcp_secret #2782

Merged
merged 2 commits into from
Jan 9, 2020
Merged

[Doc] Change sample/component/sdk documentation to not use use_gcp_secret #2782

merged 2 commits into from
Jan 9, 2020

Conversation

Bobgy
Copy link
Contributor

@Bobgy Bobgy commented Dec 30, 2019 •
edited
Loading

Changes:

This is part of #1691 (comment).
Workload identity and full scope are our recommendations, adjust sample and documentation to treat them as the default.

This change is Reviewable

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

@Bobgy
Copy link
Contributor Author

Bobgy commented Dec 30, 2019

/cc @IronPan
/assign @numerology

Can you take a look?

@k8s-ci-robot k8s-ci-robot requested a review from IronPan December 30, 2019 05:46
@Bobgy
Copy link
Contributor Author

Bobgy commented Dec 30, 2019

/cc @gaoning777
/cc @Ark-kun

@Bobgy Bobgy mentioned this pull request Dec 30, 2019
Closed
@numerology
Copy link

/lgtm
/approve
for the sample bits.

@Ark-kun
Copy link
Contributor

Ark-kun commented Jan 3, 2020

Do the modified samples support non-GKE standalone deployment?

@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 6, 2020

Do the modified samples support non-GKE standalone deployment?

No, they won't. Are there many such use cases, using non-GKE, but use GCP apis?
I hope my tutorial is good enough to let them know how to modify the samples to support that scenario. Let's improve docs if there's such a need.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jan 6, 2020
@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 6, 2020

/test kubeflow-pipeline-sample-test
building image was flaky

@Bobgy Bobgy changed the title [Sample] Change sample pipelines to not use use_gcp_secret [Sample] Change sample/component/sdk documentation to not use use_gcp_secret Jan 7, 2020
@Bobgy Bobgy changed the title [Sample] Change sample/component/sdk documentation to not use use_gcp_secret [Doc] Change sample/component/sdk documentation to not use use_gcp_secret Jan 7, 2020
@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 7, 2020

@Ark-kun Can you take a look again for sdk and component documentation update?

@gaoning777
Copy link
Contributor

/lgtm

@gaoning777
Copy link
Contributor

One more sample that might also need updating:
https://github.com/kubeflow/pipelines/blob/master/samples/tutorials/mnist/04_Reusable_and_Pre-build_Components_as_Pipeline.ipynb

@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 8, 2020

One more sample that might also need updating:
https://github.com/kubeflow/pipelines/blob/master/samples/tutorials/mnist/04_Reusable_and_Pre-build_Components_as_Pipeline.ipynb

Thanks! I read through the new tutorial, it is targetting kubeflow full deployment, so I think we can keep use_gcp_secret usage for now. We can remove it once kfp on kubeflow uses workload identity completely.

@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 8, 2020

/approve

hongye-sun
hongye-sun approved these changes Jan 8, 2020
View reviewed changes
Copy link
Contributor

@hongye-sun hongye-sun left a comment

Choose a reason for hiding this comment

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

/lgtm

@Ark-kun
Copy link
Contributor

Ark-kun commented Jan 9, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ark-kun, Bobgy, numerology

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

@gaoning777
Copy link
Contributor

One more sample that might also need updating:
https://github.com/kubeflow/pipelines/blob/master/samples/tutorials/mnist/04_Reusable_and_Pre-build_Components_as_Pipeline.ipynb

Thanks! I read through the new tutorial, it is targetting kubeflow full deployment, so I think we can keep use_gcp_secret usage for now. We can remove it once kfp on kubeflow uses workload identity completely.

Sounds good. Thanks Yuan

@k8s-ci-robot k8s-ci-robot merged commit 589aaa9 into kubeflow:master Jan 9, 2020
@Bobgy Bobgy deleted the wi_doc branch January 9, 2020 03:55
rui5i pushed a commit to rui5i/pipelines that referenced this pull request Jan 16, 2020
@Bobgy @rui5i
[Doc] Change sample/component/sdk documentation to not use `use_gcp_s…
cccbb9b 
…ecret` (kubeflow#2782)

* Update use_gcp_secret documentation to point to authenticating pipelines to GCP doc

* Update Local Development Quickstart.ipynb
Jeffwan pushed a commit to Jeffwan/pipelines that referenced this pull request Dec 9, 2020
@Bobgy @Jeffwan
[Doc] Change sample/component/sdk documentation to not use `use_gcp_s…
0143024 
…ecret` (kubeflow#2782)

* Update use_gcp_secret documentation to point to authenticating pipelines to GCP doc

* Update Local Development Quickstart.ipynb
magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023
@elukey
python: allow to override the UvicornServer's default log config (kub…
bde4f4c 
…eflow#2782)

It is useful to be able to change Uvicorn's log settings, for example
to be able to change the access log's format.

The Uvicorn's Config class provides a "configure_logging" function
that is smart about the log_config parameter - if it is a string,
and not a dictionary, it is interpreted as a file containing
logging directives and that needs to be loaded from disk.
This is very convenient for the KServe use case: one can either use
the default config, hardcoded in a dictionary, or anything
provided as yaml or json file.

This change fixes also the default UvicornServer's config.

Sadly the current Uvicorn implementation doesn't support
changing the access log format, but they suggest to use
asgi-logger:

encode/uvicorn#947 (comment)

It seems the only way forward to allow a decent customization
of a vital sign like the access log.
The main downside is that asgi-logger requires python3.8+,
so to a temporary workaround could be to add the package as extra
in poetry's config and import the AccessLoggerMiddleware only
if the user sets the access log format.

Fixes: kubeflow#2778

Signed-off-by: Luca Toscano <toscano.luca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@numerology numerology numerology left review comments

@hongye-sun hongye-sun hongye-sun approved these changes

@Ark-kun Ark-kun Awaiting requested review from Ark-kun

@gaoning777 gaoning777 Awaiting requested review from gaoning777

@IronPan IronPan Awaiting requested review from IronPan

Assignees

@gaoning777 gaoning777

@numerology numerology

@hongye-sun hongye-sun

Labels
approved area/docs area/samples/notebooks area/samples lgtm size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Bobgy @numerology @Ark-kun @gaoning777 @k8s-ci-robot @hongye-sun