Implement kfp runner with tests #359
Conversation
GeorgesLorre
requested review from
mrchtr,
RobbeSneyders and
PhilippeMoussalli
src/fondant/compiler.py
Outdated
| @@ -226,7 +226,7 @@ def _resolve_imports(self): | |||
| self.kfp = kfp | |||
| except ImportError: | |||
| msg = """You need to install kfp to use the Kubeflow compiler,\n | |||
| you can install it with `pip install --extras pipelines`""" | |||
| you can install it with `pip install --extras kfp`""" | |||
There was a problem hiding this comment.
I think the extras is still called pipelines. You can however remove the k8s dependency from it since we're no longer using it and maybe rename pipelines to kfp
There was a problem hiding this comment.
I will just update this and rename the pipelines extra to kfp in a different PR
| new=MockKfpClient, | ||
| ): | ||
| runner = KubeflowRunner(host="some_host") | ||
| runner.run(input_spec=input_spec_path) |
There was a problem hiding this comment.
What are we testing here exactly? I know it's difficult since we're mocking the pipeline run method. Can we maybe validate the run_url?
There was a problem hiding this comment.
We are testing nothing explicit but if the runner would error then the test would fail. I'll add an assert to see if the url is passed properly.
|
@PhilippeMoussalli resolved your comments |
Thanks :) good to merge |
GeorgesLorre
force-pushed
the
feature/kf-runner
branch
from
891bc87
to
bc61d0b
Compare
PR implementing the Kfp runner. Had some issues with adding the pipeline name to the kfp spec (solved now) because we need it to launch a run with a sensible name. Remarks: - removed some logic around deleting existing pipelines that we have (which was not used afaik) - I tested it on a toy pipeline and was able to push a run on the GCP kubeflow - I will implement the CLI (for both the runner and compiler) in a separate PR - I will also make a separate PR for cleaning up the kfp imports and extras.
PR implementing the Kfp runner.
Had some issues with adding the pipeline name to the kfp spec (solved now) because we need it to launch a run with a sensible name.
Remarks: