Skip to content
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

Include elyra package in all data science workbenches #58

Merged
merged 2 commits into from
Apr 7, 2023

Conversation

harshad16
Copy link
Member

@harshad16 harshad16 commented Mar 23, 2023

Include elyra package in all data science workbenches

Description

This PR includes elyra package as whole to data science workbench images.
It would bring in support for elyra pipeline, extensions and plugins.

Followed the similar pattern from contrib: https://github.com/opendatahub-io-contrib/workbench-images/tree/main/jupyter

Related-to: #55
opendatahub-io/s2i-lab-elyra#61

How Has This Been Tested?

To be tested from PR images.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@atheo89
Copy link
Member

atheo89 commented Mar 24, 2023

Awesome! I have just one observation to make. Would it be possible to hide the /utils/setup-elyra.sh from the /opt/app-root/src workdir? Perhaps you could adopt the approach used in the minimal dockerfile.

Screenshot from 2023-03-24 12-41-20

@harshad16
Copy link
Member Author

Thanks @atheo89 , i will fix that 👍

@harshad16 harshad16 changed the title Include elyra package in all data science workbenches [wip] Include elyra package in all data science workbenches Mar 24, 2023
@harshad16 harshad16 force-pushed the setup-elyra branch 4 times, most recently from b1d564f to a3a283a Compare March 28, 2023 13:08
@harshad16 harshad16 changed the title [wip] Include elyra package in all data science workbenches Include elyra package in all data science workbenches Mar 28, 2023
@harshad16
Copy link
Member Author

Ready for Review:
Include elyra plugins:
pipeline-editor, python-editor and code-snippet
reference: #55 (comment)

The plugin are working in the image:
Screenshot from 2023-03-28 09-24-48

@harshad16
Copy link
Member Author

/test notebooks-e2e-tests

1 similar comment
@atheo89
Copy link
Member

atheo89 commented Mar 29, 2023

/test notebooks-e2e-tests

# Install Python packages and Jupyterlab extensions from Pipfile.lock
COPY Pipfile.lock ./

# Copy Elyra setup to utils so that it's sourced at startup
COPY setup-elyra.sh ./utils/

RUN echo "Installing softwares and packages" && micropipenv install && rm -f ./Pipfile.lock

Copy link
Member

@atheo89 atheo89 Mar 29, 2023

Choose a reason for hiding this comment

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

It seems that the Python version displayed on the launcher is not inherited from minimal.
Could you please add the following command on the data-science dockerfiles 3.8/3.9?

# Replace Notebook's launcher, "(ipykernel)" with Python's version 3.x
RUN sed -i -e "s/Python.*/$(python --version | awk '{print "Python", $2}' | cut -d'.' -f-2)\",/" /opt/app-root/share/jupyter/kernels/python3/kernel.json

Moreover, update the corresponding command into the minimal dockerfile, and remove it from the TrustyAi dockerfile, because it will be inherited by the datascience dockerfile.

This cmd displays the python version without the patch version, so eventually, we could close also this issue: #49

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like this cmd is needed explicitly in each image build.
As the jupyter setup is getting overridden in build-time, the base image change of the kernel version is not carried over.
Tested on different images.
Included it in another images as well.

Copy link
Member

Choose a reason for hiding this comment

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

This one is not implemented yet.
(Maybe we can leave it for another time, in an enhancement pr)

Copy link
Member Author

Choose a reason for hiding this comment

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

i didnt understand the comment,
would like me to remove the change made and move it separate pr for later ?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, let's leave it for later on, to unblock this pr

@atheo89
Copy link
Member

atheo89 commented Mar 29, 2023

Moreover, while I was browsing into the files, I observed that the pyTorch 3.9 notebook, has wrong references on the Lebels cmd: https://github.com/harshad16/odh-notebooks/blob/setup-elyra/jupyter/pytorch/ubi9-python-3.9/Dockerfile#L4-L12 Could you update the those to 3.8 to 3.9?
Additionally, to that, I realized the io.openshift.build.image label in all the dockefiles points to the old registry, but should points to quay.io/opendatahub/workbench-images:${NOTEBOOK_IMAGE} 😆 .
I know that these updates don't relate to that particular pr, but if it is easy for you could you update them, otherwise we could open another PR later for it.

@harshad16 harshad16 force-pushed the setup-elyra branch 2 times, most recently from e50bd97 to 82e4402 Compare March 30, 2023 13:07
@harshad16
Copy link
Member Author

/test notebooks-e2e-tests

@harshad16
Copy link
Member Author

/test notebook-cuda-ubi8-python-3-8-pr-image-mirror

1 similar comment
@harshad16
Copy link
Member Author

/test notebook-cuda-ubi8-python-3-8-pr-image-mirror

@harshad16
Copy link
Member Author

/retest

1 similar comment
@harshad16
Copy link
Member Author

/retest

@harshad16 harshad16 changed the title Include elyra package in all data science workbenches [wip] Include elyra package in all data science workbenches Apr 3, 2023
@harshad16
Copy link
Member Author

/test images

@harshad16
Copy link
Member Author

/test notebooks-e2e-tests

@harshad16 harshad16 changed the title [wip] Include elyra package in all data science workbenches Include elyra package in all data science workbenches Apr 3, 2023
jupyter/datascience/ubi9-python-3.9/Dockerfile Outdated Show resolved Hide resolved
# Install Python packages and Jupyterlab extensions from Pipfile.lock
COPY Pipfile.lock ./

# Copy Elyra setup to utils so that it's sourced at startup
COPY setup-elyra.sh ./utils/

RUN echo "Installing softwares and packages" && micropipenv install && rm -f ./Pipfile.lock

Copy link
Member

Choose a reason for hiding this comment

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

This one is not implemented yet.
(Maybe we can leave it for another time, in an enhancement pr)

jupyter/datascience/ubi8-python-3.8/Dockerfile Outdated Show resolved Hide resolved
- Filter out Pipeline plugin only for local and kfp
- Pre-set the kfp-tekton for kfp pipeline support

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>
Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>
@atheo89
Copy link
Member

atheo89 commented Apr 7, 2023

/lgtm

@VaishnaviHire
Copy link
Member

/lgtm

Tested this by running an example pipeline

Screenshot 2023-04-07 at 11 00 14 AM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: VaishnaviHire

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Apr 7, 2023
@harshad16
Copy link
Member Author

/test notebooks-e2e-tests

@openshift-merge-robot openshift-merge-robot merged commit e95157b into opendatahub-io:main Apr 7, 2023
harshad16 added a commit to harshad16/odh-notebooks that referenced this pull request May 9, 2023
)

* Include elyra package in all data science workbenches

- Filter out Pipeline plugin only for local and kfp
- Pre-set the kfp-tekton for kfp pipeline support

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

* Point all images label at opendatahub/workbench-images repo

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

---------

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>
harshad16 added a commit to harshad16/odh-notebooks that referenced this pull request May 9, 2023
)

* Include elyra package in all data science workbenches

- Filter out Pipeline plugin only for local and kfp
- Pre-set the kfp-tekton for kfp pipeline support

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

* Point all images label at opendatahub/workbench-images repo

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

---------

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>
harshad16 added a commit to harshad16/odh-notebooks that referenced this pull request May 9, 2023
)

* Include elyra package in all data science workbenches

- Filter out Pipeline plugin only for local and kfp
- Pre-set the kfp-tekton for kfp pipeline support

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

* Point all images label at opendatahub/workbench-images repo

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

---------

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>
openshift-merge-robot pushed a commit that referenced this pull request May 9, 2023
* Include as a reviewer for workbench images (#52)

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

* Include elyra package in all data science workbenches (#58)

* Include elyra package in all data science workbenches

- Filter out Pipeline plugin only for local and kfp
- Pre-set the kfp-tekton for kfp pipeline support

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

* Point all images label at opendatahub/workbench-images repo

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

---------

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

* Add documentation for notebooks image update process (#61)

* updated trustyai to 0.2.10

* Update Owners file (#66)

* updated trustyai to 0.2.11

* Include runtime images into workbench images

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

* Included the validation test for runtime images

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

* updated trustyai to 0.2.12

* Wait for the runtime pod readiness for e2e tests (#76)

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

* Remove the default Pipeline runtime config (#70)

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

* Provide runtimes image in workbench images (#64)

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

* Allow elyra pipeline config mount on ~/runtimes dir (#65)

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

* Set default env vars for ssl_sa_certs and sa_token path (#77)

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>

---------

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>
Co-authored-by: aTheo <atheodorak@outlook.com>
Co-authored-by: Tommaso Teofili <tteofili@redhat.com>
Co-authored-by: Vaishnavi Hire <vhire@redhat.com>
@harshad16 harshad16 deleted the setup-elyra branch May 11, 2023 10:03
@shalberd
Copy link

Elyra has been taken over by RedHat, and RedHat in turn is a part of IBM, so Redhat will now also take part of discussions at elyra-ai.slack.com?

Regarding this PR: That is very cool! @harshad16 So there is no more need for an explicit Elyra notebook image via overlay? https://github.com/opendatahub-io/odh-manifests/tree/master/notebook-images/overlays/additional

@harshad16
Copy link
Member Author

Elyra has been taken over by RedHat, and RedHat in turn is a part of IBM, so Redhat will now also take part of discussions at elyra-ai.slack.com?

we will take part in the slack.

Regarding this PR: That is very cool! @harshad16 So there is no more need for an explicit Elyra notebook image via overlay? https://github.com/opendatahub-io/odh-manifests/tree/master/notebook-images/overlays/additional

yes , the eventual goal is to remove the overlay
our plan is to wait for one release and then once it announce and user have time to switch to new images
we would remove the overlay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants