Skip to content
This repository has been archived by the owner on Mar 11, 2022. It is now read-only.

ODH crio fix #56

Merged
merged 1 commit into from
Oct 22, 2020
Merged

ODH crio fix #56

merged 1 commit into from
Oct 22, 2020

Conversation

akchinSTC
Copy link
Member

Updates the kfp-notebook to support pip installation from the notebook
and aligns pipeline runtime environment with the elyra image in odh

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@akchinSTC akchinSTC force-pushed the odh-crio-fix branch 3 times, most recently from 5910fe4 to 5645636 Compare October 8, 2020 18:13
@akchinSTC
Copy link
Member Author

There exists a current issue right now with invoking papermill via subprocess.
The papermill script that normally gets installed to /usr/local/bin is redirected and installed into mounted volume however attempts to invoke it result in a permissions error. Tunneling into the running container, running papermill via the script functions without issue. User permissions were identical so there is something else going on. Quickest workaround is invoking papermill via the python lib (like prior to refactor) ...its not the prettiest though.

@akchinSTC akchinSTC force-pushed the odh-crio-fix branch 3 times, most recently from f329d0c to 3ebfabb Compare October 22, 2020 04:29
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Alan, just had the two comments and a question.

Is the user_volume_path logic solely introduced for read-only container systems like crio or would this same logic hold for docker-based container systems? I'm just wondering if we should change its name or add another attribute to indicate behavior unique for crio, if that's the case.

etc/docker-scripts/bootstrapper.py Outdated Show resolved Hide resolved
kfp_notebook/pipeline/_notebook_op.py Show resolved Hide resolved
Copy link
Member

@lresende lresende left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@akchinSTC
Copy link
Member Author

Thanks Alan, just had the two comments and a question.

Is the user_volume_path logic solely introduced for read-only container systems like crio or would this same logic hold for docker-based container systems? I'm just wondering if we should change its name or add another attribute to indicate behavior unique for crio, if that's the case.

@kevin-bates - The user-volume logic was introduced specifically as a workaround for crio. However if you did run with the crio_runtime flag enabled on a docker based container system, it would still run...same destination with a few more extra steps. We could change the naming to be more specific to crio if it would help with distinction. Only thing is that if another runtime, like containerd?(dont know havent tested), requires the same logic, we will need to rename.

Sidenote: argoproj/argo-workflows#2679 (comment) - This just went in this morning addressing the emptydir issue with argo, which was the root of our problem here. So things may change again once kubeflow picks up the release with this fix.

@kevin-bates
Copy link
Member

Thanks Alan - this seems fine and it sounds like the field is still too unknown anyway. Let's let things play out as is for the time being.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Previously when installing python packages within the notebook e.g. using
'!pip install' the packages failed to install due to write permissions.
This sets the target directory path to the mounted volume for
all pip installations via a global pip configuration file.
This volume is also mounted to same writable root path /opt/app-root/src/
to align with the ODH jupyterhub environment when using the s2i-lab-elyra
image.
@akchinSTC akchinSTC merged commit c3d202f into elyra-ai:master Oct 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants