-
Notifications
You must be signed in to change notification settings - Fork 18
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
s2i: Add img support #33
Conversation
Signed-off-by: Ce Gao <[email protected]>
Signed-off-by: Ce Gao <[email protected]>
/assign @ddysher |
package img | ||
|
||
const ( | ||
tensorflowTemplate = `FROM tensorflow/tensorflow:1.10.1-py3 |
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'm not sure it will always work; most of the time, we need a lot libraries other than tensorflow. Do you have any solution in mind? I think this is not specific to ciao, but a general question.
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.
In the current stage, I prefer all-in-one support.
In the future, I think we should support adding new layers in the image to add dependencies while I have no detailed design for it since it is out of the scope of ciao now.
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.
rough approaches:
- analyze python source code and install imported packages, or use
%package xxx
syntax (slow) - wrap add all commonly used packages in a base image (huge image)
- others..
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 created an issue for it. #35
return "", err | ||
} | ||
|
||
cmd := exec.Command("img", "build", "-t", imageName, dir) |
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.
here we need to pre-install img
? what if notebook itself is running in a pod?
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.
Then img should be added into the image, with runc and some other deps.
/assign @ddysher PTAL, I think we can merge the PR and implement other features in incoming PRs |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ddysher, gaocegege 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 |
What this PR does / why we need it:
img support added
Which issue(s) this PR is related to (optional, link to 3rd issue(s)):
Special notes for your reviewer:
/cc @ddysher
Release note: