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

Bump KFP to 0.2.5 #1083

Merged
merged 5 commits into from
Apr 9, 2020
Merged

Bump KFP to 0.2.5 #1083

merged 5 commits into from
Apr 9, 2020

Conversation

Bobgy
Copy link
Contributor

@Bobgy Bobgy commented Apr 8, 2020 •
edited
Loading

Which issue is resolved by this Pull Request:
Resolves #984
Part of kubeflow/kubeflow#4929

Description of your changes:
Bump KFP to 0.2.5

Checklist:

  • Unit tests have been rebuilt:
    1. cd manifests/tests
    2. make generate-changed-only
    3. make test

@kubeflow-bot
Copy link
Contributor

This change is Reviewable

@Bobgy
Copy link
Contributor Author

Bobgy commented Apr 8, 2020 •
edited
Loading

I cannot build manifest tests. I got the following error when running make generate-changed-only:

INFO|2020-04-08T16:44:34|../hack/generate_tests.py|47| Creating directory /Users/gongyuan/kfp/manifests/tests/tests/legacy_kustomizations/centraldashboard/test_data/expected
Error: merging from generator &{0xc00003a540 0xc00011abb8 {map[] map[] false} {{ parameters merge {[] [] [params_0.env] }}}}: cannot find resource with id ~G_v1_ConfigMap|~X|parameters to replace
Traceback (most recent call last):
  File "../hack/generate_tests.py", line 188, in <module>
    run_kustomize_build(repo_root, full_dir)
  File "../hack/generate_tests.py", line 52, in run_kustomize_build
    package_dir))
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 190, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['kustomize', 'build', '--load_restrictor', 'none', '-o', u'/Users/gongyuan/kfp/manifests/tests/tests/legacy_kustomizations/centraldashboard/test_data/expected']' returned non-zero exit status 1

@richardsliu @jlewi any clue what might be wrong?

I'm using kustomize v2.1.0.
Tried make generate in master branch too, got same error.

@Bobgy
Copy link
Contributor Author

Bobgy commented Apr 8, 2020

/assign @richardsliu

@richardsliu
Copy link
Contributor

richardsliu commented Apr 8, 2020 •
edited
Loading

@Bobgy Please take a look at this issue for the fix: kubeflow/metadata#202

Specifcally, in kustomization.yaml, change

 configMapGenerator:
   name: ui-parameters
   env: params.env

to:

 configMapGenerator:
   name: ui-parameters
   envs: 
      - params.env

@Bobgy
Copy link
Contributor Author

Bobgy commented Apr 9, 2020

@richardsliu thanks, so we use kustomize v3.
Okay, I will fix the manifest

@Bobgy
Copy link
Contributor Author

Bobgy commented Apr 9, 2020

@richardsliu I updated and fixed the test. PTAL again

@k8s-ci-robot k8s-ci-robot added size/L and removed size/S labels Apr 9, 2020
@Bobgy
Copy link
Contributor Author

Bobgy commented Apr 9, 2020

Actually, there are still some problems when KFP runs.

  • tfx example cannot get metadata-grpc-configmap in kubeflow namespace, because it is actually named metadata-metadata-grpc-configmap-2dd6h7mhg6 in kubeflow. (common prefix metadata- added the duplicate prefix, and configmap generator added the hash, can we drop them?)

@Bobgy
Copy link
Contributor Author

Bobgy commented Apr 9, 2020

@zhenghuiwang @jlewi please review the metadata grpc configmap change

Bobgy
Bobgy commented Apr 9, 2020
View reviewed changes
tests/legacy_kustomizations/metadata/kustomization.yaml
@@ -17,7 +17,7 @@ configMapGenerator:
- behavior: merge
envs:
- params_1.env
name: metadata-grpc-configmap
name: grpc-configmap
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this expected? I had to manually fix this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be right. I remember the name has to be metadata-grpc-configmap.
cc @dushyanthsc

@Bobgy
Copy link
Contributor Author

Bobgy commented Apr 9, 2020

Added one more change to fix visualization server GCP permission.

@Bobgy
Copy link
Contributor Author

Bobgy commented Apr 9, 2020

Okay, I think I got all basic features integrated by default.

  • all samples (including a tfx pipeline)
  • tfx visualization
  • tensorboard
  • pipeline run/retry

@Bobgy
Copy link
Contributor Author

Bobgy commented Apr 9, 2020

This is ready for complete review

zhenghuiwang
zhenghuiwang reviewed Apr 9, 2020
View reviewed changes
Copy link
Contributor

@zhenghuiwang zhenghuiwang left a comment

Choose a reason for hiding this comment

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

I remember @dushyanthsc added the grpc config map and TFX has some ENV variable hardcoded as metadata-config-map to find the server info.

/cc @dushyanthsc can you review?

tests/legacy_kustomizations/metadata/kustomization.yaml
@@ -17,7 +17,7 @@ configMapGenerator:
- behavior: merge
envs:
- params_1.env
name: metadata-grpc-configmap
name: grpc-configmap
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be right. I remember the name has to be metadata-grpc-configmap.
cc @dushyanthsc

@zhenghuiwang
Copy link
Contributor

@Bobgy If you have verified TFX works then I think it is good to be merged.

/assign @dushyanthsc

@zhenghuiwang
Copy link
Contributor

/lgtm

It correctly generated configmap/metadata-grpc-configmap with kustomize 2.0.3

@richardsliu
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardsliu

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

@k8s-ci-robot k8s-ci-robot merged commit f263eb3 into kubeflow:master Apr 9, 2020
@jlewi jlewi mentioned this pull request Apr 10, 2020
Merged
1 task
@Bobgy Bobgy deleted the kfp_0.2.5 branch April 10, 2020 00:47
@Bobgy
Copy link
Contributor Author

Bobgy commented Apr 10, 2020

Thanks @richardsliu @zhenghuiwang for review!

Note, metadata kustomization.yaml specified commonPrefix as metadata-, so I needed to drop each occurences of extra metadata- from grpc-configmap.

Bobgy added a commit to Bobgy/manifests that referenced this pull request Apr 10, 2020
@Bobgy
Cherry pick of kubeflow#1083 bump kfp to 0.2.5
4ece5ed
k8s-ci-robot pushed a commit that referenced this pull request Apr 10, 2020
@Bobgy
Cherry pick of #1083 Bump KFP to 0.2.5 (#1093)
1c00235 
* Cherry pick of #1083 bump kfp to 0.2.5

* Regenerate tests

* fix test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhenghuiwang zhenghuiwang zhenghuiwang left review comments

@IronPan IronPan Awaiting requested review from IronPan

@krishnadurai krishnadurai Awaiting requested review from krishnadurai

Assignees

@zhenghuiwang zhenghuiwang

@richardsliu richardsliu

@dushyanthsc dushyanthsc

Labels
approved cla: yes lgtm size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bump pipeline version to 0.2.5 in v1.0-branch
7 participants
@Bobgy @kubeflow-bot @richardsliu @zhenghuiwang @k8s-ci-robot @googlebot @dushyanthsc