Migrate wbuchwalter/fairing #5
Conversation
skeleton for serving
Refactor strategies/architectures
Update README
Add namespace support
|
/approve |
|
@jlewi I will let you merge this one once you have reviewed that licensing is correct. I am working on a fix. I will open a PR here once this one is merged. Also not that @ashahba has been working on a PR to add a new builder using configmaps (faster build than using Knative, but less flexible wbuchwalter/fairing#27). This PR will be transferred here as well so it can be discussed by the community. |
There was a problem hiding this comment.
Awesome work @wbuchwalter, thanks for the contribution. I took a first pass at reviewing this and I just have a few questions on the overall design. Is there is a design document somewhere?
| echo "FAIRING_DEV_DOCKER_USERNAME is unset. Set it to your docker username to use this command"; \ | ||
| else \ | ||
| pip install . ;\ | ||
| docker build -t ${FAIRING_DEV_DOCKER_USERNAME}/fairing:latest -f Dockerfile.dev . ; \ |
There was a problem hiding this comment.
I have some ideas on how we could get rid of scripts like this, but for now you might want to avoid pushing anything with "latest" and tag with git commit sha.
There was a problem hiding this comment.
This specific script is to make easier to help developers try their changes without having to go through the pip release process.
You would set FAIRING_DEV_DOCKER_USERNAME to your username and this would push a dev version to your dockerhub. This image is then pulled instead of the regular base image.
So latest is needed so that you don't have to go modify this script on every change.
| See: [Installing Knative Build Component](https://github.com/knative/docs/blob/master/build/installing-build-component.md) | ||
|
|
||
|
|
||
| ### Container Registry Secret |
There was a problem hiding this comment.
I think that we should reuse the user service account that gets created instead
There was a problem hiding this comment.
I'm not sure I understand with service account you are referring to?
|
|
||
| Kubeflow needs to be pre-installed in the cluster. | ||
|
|
||
| ### Knative Build Component |
There was a problem hiding this comment.
I think that Knative is a heavy dependency to build images on the cluster. If we're just appending python files, we can do it much simpler and quicker without knative.
There was a problem hiding this comment.
Knative is the solution that came out after discussing with @jlewi.
You can see some of the context for that in this thread: kubeflow/kubeflow#1240 (comment)
Note that @ashahba has a PR opened on the old repo to use configmaps directly and bypass the build step entirely (wbuchwalter/fairing#27). I ll move this PR here once this one get merged. However this approach does not allow for a lot of flexibility since you cannot customize the remote environment.
Curious to hear your idea.
| self.ps_count = ps_count | ||
| self.worker_count = worker_count | ||
|
|
||
| def add_jobs(self, svc, count, img, name, volumes, volume_mounts): |
There was a problem hiding this comment.
WDYT about using the k8s clients directly here?
| return svc | ||
|
|
||
| def stream_logs(self, image_name, image_tag): | ||
| mp_client = MetaparticleClient() |
There was a problem hiding this comment.
Why the dependency on metaparticle? It seems a bit unnecessary
There was a problem hiding this comment.
Context for metaparticle here: kubeflow/kubeflow#1240 (comment).
However I am not convinced anymore that this is the right approach since metaparticle is not maintained it seems.
We can remove it in a later PR.
|
@r2d4 Addressed some of your comments. What do you think? |
|
Yep SGTM. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: r2d4, wbuchwalter 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 |
Follow up to #2 with correct licence change applied.
This change is