[WIP] Add ConfigMap builder #21
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: If they are not already assigned, you can assign the PR to them by writing 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 |
|
|
||
| def __init__(self, notebook_name=None): | ||
| if notebook_name is None: | ||
| self.notebook_name = notebook_helper.get_notebook_name() |
There was a problem hiding this comment.
Does this work? If not, we should make notebook_name required.
There was a problem hiding this comment.
It works in standalone notebooks, not with JupyterHub.
I can improve the error handling in the helper to display an explicit error when user JH such as: "notebook_name is required when using JupyterHub"
| client.V1ConfigMap( | ||
| api_version="v1", | ||
| data={"code.py": code}, | ||
| metadata=client.V1ObjectMeta(name=self.job_id) |
There was a problem hiding this comment.
Is job_id a parameter or member? Who sets it?
There was a problem hiding this comment.
job_id is generated automatically by the Deployment and is used to ensure we get the correct logs.
| k8s.create_config_map(namespace, config_map) | ||
| return self._generate_pod_spec(job_id, base_image) | ||
|
|
||
| def _generate_pod_spec(self, job_id, base_image): |
There was a problem hiding this comment.
Not sure if it would be this PR but would be great to provide this pod-spec using a configmap(that way specifying images, environments, and other mounts is completely upto the user).
There was a problem hiding this comment.
This would be part of the discussion here: #28 and be in it's own PR.
|
@wbuchwalter Any update on this? |
|
@jlewi Still not sure about this one. I would like to avoid having a ton of different builders if we can converge on one. |
|
Can we avoid image building with the append builder? Our issue is with
image building.
…On Mon, Jan 7, 2019, 8:02 AM William Buchwalter ***@***.*** wrote:
@jlewi <https://github.com/jlewi> Still not sure about this one. I would
like to avoid having a ton of different builders if we can converge on one.
@ashahab <https://github.com/ashahab>, as far as I know your use case is
the only one requiring this. I would like confirmation that the append
builder definitly cannot suit your needs.
If not, then would there be any drawback to publish this builder as an
external package? Having this on an external repo and as an additional pip
package?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#21 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAqJPRmLUefc796CKYk2hekhX7OVmkFJks5vA2-igaJpZM4Yv4bl>
.
|
|
I think this is obsolete. |
Re-implementation of #15 following the refactoring in #20.
cc @ashahab
In this current state, there is now way to specify different base images for worker and ps.
I will address that in a separate PR, as this need to be solved for all builders not just this one.
This PR also contains some unrelated changes such as renaiming
nativeinkubernetes.This change is