-
Notifications
You must be signed in to change notification settings - Fork 722
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.
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
Lock in version of kfp
#30
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
I signed it |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix @MattMorgis!
I'll also send out a fix later so that later versions of KFP can be used as well.
PiperOrigin-RevId: 242708530
Thanks for the fix. The runner is failing, because it was using undocumented hack. The KFP documentation clearly says that the pipeline function must be decorated with Running tfx/tfx/orchestration/kubeflow/runner.py Line 125 in d9f87b9
'Please use a function with @dsl.pipeline decorator.' .
This documented behavior was circumvented by a hack that used an undocumented function that should be a part of internal compiler API. Ning has recently changed this internal function, which triggered TFX failure. I'll send a PR fixing TFX code fixing this issue and removing the use of undocumented API. |
See tensorflow#30 (comment) The KFP documentation clearly says that the pipeline function must be decorated with `@pipeline` decorator. It's even mentioned in the `compile` function documentation: https://github.com/kubeflow/pipelines/blob/086d4763d9f163acdeb423bc2bc49ab470442a92/sdk/python/kfp/compiler/compiler.py#L636 Running `compile` without having the function decorated https://github.com/tensorflow/tfx/blob/d9f87b9b03ab8bb98653b8a96d5bf952bc22b2b7/tfx/orchestration/kubeflow/runner.py#L125 will raise error: https://github.com/kubeflow/pipelines/blob/086d4763d9f163acdeb423bc2bc49ab470442a92/sdk/python/kfp/compiler/compiler.py#L578 `'Please use a function with @dsl.pipeline decorator.'`. For some reason this documented behavior was circumvented by a hack that used an undocumented function that should be a part of internal compiler API. This PR fixes this issue.
Added the PR to fix the TFX code: #39 |
Merging accepted RFC.
My team and I hit a snag this week when trying to compile Kubeflow Pipelines:
Thanks to commit
eb63dae
this week which added a test for this, we were able to debug easier.It turns out the issue was that we had version
0.1.14
of the Kubeflow Pipelines SDK installed. The docs recommended this, and the docs and the Release Page do not install from PyPi.This feels like it was intentional, but only version published to PyPi is
0.1.11
, the last compatible version with tfx. I've tried0.1.12
all the way to the most recent0.1.15
and continue to get the same error as above.Additionally, the check submitted by this PR doesn't even catch this.
When running
pip show kfp
from the non-PyPi install, I get:And when running after installing from
setup.py
:I'm not sure if I should follow an issue in Kubeflow/pipelines and ask if they can tag with the patch, or if these will never be published to PyPi and this is a non-issue.
I'm unsure what the proper fix should be, but wanted to submit to start the conversation and document for anyone else who may run into this.