-
Notifications
You must be signed in to change notification settings - Fork 138
chore(gha): Create E2E test that runs with KFP #532
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
base: main
Are you sure you want to change the base?
Conversation
jesuino
left a comment
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.
Adding a comment about letting KFP access Kale latest version.
hmtosi
left a comment
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.
Great work overall Adam, thank you for writing the test! I just have a few clarifying questions to start, and then I can test the script and do my final review
| default: "true" | ||
| kale-installation-string: | ||
| required: false | ||
| description: "Installation string for kale, only used if use-local-kale is false (e.g. dev-kubeflow-kale or kubeflow-kale==0.7.0)" |
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.
For clarity, I would change this to Installation string for kale (e.g. dev-kubeflow-kale or kubeflow-kale==0.7.0). Only used if use-local-kale is False.
| else | ||
| pip install $KALE_INSTALLATION_STRING | ||
| fi | ||
| echo "KALE_PIP_INDEX_URLS=https://test.pypi.org/simple" >> $GITHUB_ENV |
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.
This is the url for test PyPI, which is where dev-kubeflow-kale is published, but would it also need https://pypi.org/simple for kubeflow-kale?
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.
I don´t think so.
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.
@hmtosi the production pypi is added anyway:
https://github.com/kubeflow-kale/kale/blob/main/backend/kale/common/utils.py#L242
| --pipeline_name e2e-test \ | ||
| --pipeline_descr "E2E Test" | ||
| sed -i "s/'kubeflow-kale[^']*'/'dev-kubeflow-kale'/g" .kale/e2e-test.kale.py |
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.
Doesn't this cause it to automatically run the dev-kubeflow-kale version no matter what is set in the installation string?
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.
This must be changed, You could reuse the existing kale version parameter or have a new kale_runtime_package_version that would specify only the runtime kale version to be used when running the pipeline on KFP.
jesuino
left a comment
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.
Would you please change the code mentioned by @hmtosi ? You can either use a new parameter to specify the runtime kale version or reuse the existing one. Or even build from the sources
@StefanoFioravanzo Would you please share your thoughts about this? Right now Adam is using a published artifact, but he figured out a way to use Kale in development and publish its artifacts to devpi to be used by kind, however, this seems to require quite a few code changes. Please let us know if you think that using a configurable runtime KFP Kale package is good enough or if we should build Kale and use it with KFP.
Thanks!
ederign
left a comment
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.
@ada333 I'll need to take a deep look on this to see what will be our strategy now that we are migrating to uv. But I'll review this as soon as I figure out a plan for how uv (and our artifacts should be used).
This PR adds a GHA that installs kfp on kind cluster and runs the base Kale example (candies) and checks if run was successful.
There is boolean option either to use a local kale version - it installs kale like this:
cd backend && pip install -e .[dev]Another option is to use official kale version and to use "installation string", right now the action works for dev-kubeflow-kale, with this installation string:
--extra-index-url https://test.pypi.org/simple/ dev-kubeflow-kaleThis test currently works only for dev-kubeflow-kale.