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 script on workbench to copy runtime-image json to relevant path #909

Merged
merged 1 commit into from
Mar 5, 2025

Conversation

atheo89
Copy link
Member

@atheo89 atheo89 commented Feb 20, 2025

Related to: https://issues.redhat.com/browse/RHOAIENG-20241

Description

This PR include the following changes:

  • Modifies the elyra.sh script to copy the content from mount path /opt/app-root/pipeline-runtimes/ to elyra path /opt/app-root/share/jupyter/metadata/runtime-images/ this take place when the DS notebook starts up. The reason is to avoid override the runtime images that the user may have import by himself.

(quay.io/rh_ee_atheodor/workbench-images@sha256:f2bc105afab97575007a5743e39cc4c045fa276ba8612b6a8241758d48711913) custom_v3

How Has This Been Tested?

This changes have been tested along with notebook controller (PR soon) by importing the new data science image generated by this PR

These are the logs when the datachience notebook when it get spin up on cluster.
The list of .json comes from the mount assigned on the notebook from the notebook-controller
image

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

@openshift-ci openshift-ci bot requested review from jiridanek and paulovmr February 20, 2025 09:44
@openshift-ci openshift-ci bot added size/m and removed size/m labels Feb 20, 2025
@atheo89 atheo89 requested a review from harshad16 February 20, 2025 11:42
@openshift-ci openshift-ci bot added size/m and removed size/m labels Feb 20, 2025
@openshift-ci openshift-ci bot added size/m and removed size/m labels Feb 20, 2025
@openshift-ci openshift-ci bot added size/xs and removed size/m labels Feb 20, 2025
@atheo89 atheo89 changed the title Remove json runtime files from the data science notebook Include script on workbench to copy runtime-image json to relevant path Feb 20, 2025
@openshift-ci openshift-ci bot added size/xs and removed size/xs labels Feb 20, 2025
@atheo89
Copy link
Member Author

atheo89 commented Feb 20, 2025

The runtime .json files will be removed later on a separated PR, basically when we will be ready to include to the project the mounting from the odh-notebook-controller

@openshift-ci openshift-ci bot added size/xs and removed size/xs labels Feb 24, 2025
Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

great work,

Shall we put this work , under dont-merge
till this PR is looked into:opendatahub-io/kubeflow#513 ?

if not, lets add back the stuff that is removed in this PR, and we can merge this and later remove the code, when PR 513 is merged.

@openshift-ci openshift-ci bot added the size/xs label Mar 4, 2025
@openshift-ci openshift-ci bot added size/xs and removed size/xs labels Mar 4, 2025
@jstourac
Copy link
Member

jstourac commented Mar 4, 2025

one comment, otherwise LGTM

we may want to create a simply pytest test for this ensuring that the set of expected json files for the runtime images are placed on the expected path - can be done as a separate task I suppose

@jiridanek
Copy link
Member

not sure if pytest test would be particularly helpful for this, maybe

/lgtm

@jiridanek jiridanek added the lgtm label Mar 4, 2025
Copy link
Contributor

openshift-ci bot commented Mar 4, 2025

@atheo89: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/notebook-rocm-jupyter-pyt-ubi9-python-3-11-pr-image-mirror b23bb0c link true /test notebook-rocm-jupyter-pyt-ubi9-python-3-11-pr-image-mirror
ci/prow/rocm-notebooks-e2e-tests b23bb0c link true /test rocm-notebooks-e2e-tests
ci/prow/notebook-rocm-jupyter-tf-ubi9-python-3-11-pr-image-mirror b23bb0c link true /test notebook-rocm-jupyter-tf-ubi9-python-3-11-pr-image-mirror

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@jstourac
Copy link
Member

jstourac commented Mar 4, 2025

/lgtm

@jstourac
Copy link
Member

jstourac commented Mar 4, 2025

not sure if pytest test would be particularly helpful for this, maybe

@jiridanek what do you suggest instead?

@jiridanek
Copy link
Member

what do you suggest instead

merge and pray

no, seriously, I think nothing lesser than full e2e with odh-notebook-controller is helpful here, plus maybe playwright to open up the runtimes in elyra afterwards

anything less does not really check anything useful, seems to me; but if i was shown pytest test, maybe i'd be proven wrong

@jstourac
Copy link
Member

jstourac commented Mar 4, 2025

no, seriously, I think nothing lesser than full e2e with odh-notebook-controller is helpful here, plus maybe playwright to open up the runtimes in elyra afterwards

anything less does not really check anything useful, seems to me; but if i was shown pytest test, maybe i'd be proven wrong

okay, no objections - my point was to have at least something. Having this in e2e is even better. Thus with our current approach that would be opendatahub/tests repo.

@jiridanek
Copy link
Member

Having this in e2e is even better

way less convenient for quickly fixing stuff during development

@atheo89
Copy link
Member Author

atheo89 commented Mar 4, 2025

we may want to create a simply pytest test for this ensuring that the set of expected json files for the runtime images are placed on the expected path - can be done as a separate task I suppose

@jstourac The cp/mounting process occurs when the notebook starts up within the RHOAI environment, as the notebook-controller incorporates the config map. So, as Jiri mentioned, we may need to test this behavior there.

@jstourac
Copy link
Member

jstourac commented Mar 4, 2025

@jstourac The cp/mounting process occurs when the notebook starts up within the RHOAI environment, as the notebook-controller incorporates the config map. So, as Jiri mentioned, we may need to test this behavior there.

Yep, I'm aware 🙂 My original idea for the proposal here was to utilize the current ability of us to run our pytest tests against the kubernetes cluster, so I though about having a notebook controller in place on that cluster for this test. There is very simple check that creates kubernetes instance in GHA already, so we would need to extend this configuration properly.

@atheo89
Copy link
Member Author

atheo89 commented Mar 5, 2025

There is very simple check that creates kubernetes instance in GHA already, so we would need to extend this configuration properly.

Looks fair approach to me.

@atheo89
Copy link
Member Author

atheo89 commented Mar 5, 2025

Thanks for you review guys i will move this in.

/approve

Copy link
Contributor

openshift-ci bot commented Mar 5, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atheo89

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 Mar 5, 2025
@jiridanek
Copy link
Member

/override ci/prow/notebooks-ubi9-e2e-tests

cluster provisioning seems broken, https://redhat-internal.slack.com/archives/C060A5FJEAD/p1741153298133919

Copy link
Contributor

openshift-ci bot commented Mar 5, 2025

@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/notebooks-ubi9-e2e-tests

In response to this:

/override ci/prow/notebooks-ubi9-e2e-tests

cluster provisioning seems broken, https://redhat-internal.slack.com/archives/C060A5FJEAD/p1741153298133919

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@jstourac
Copy link
Member

jstourac commented Mar 5, 2025

/override ci/prow/images

Copy link
Contributor

openshift-ci bot commented Mar 5, 2025

@jstourac: Overrode contexts on behalf of jstourac: ci/prow/images

In response to this:

/override ci/prow/images

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-merge-bot openshift-merge-bot bot merged commit b212877 into opendatahub-io:main Mar 5, 2025
14 checks passed
@harshad16
Copy link
Member

This PR has been rebased and it is ready for a final review.

Shall we put this work , under dont-merge
till this PR is looked into:opendatahub-io/kubeflow#513 ?

@harshad16 it is save to be merged, as i don't remove the: jupyter/datascience/ubi9-python-3.11/runtime-images/*.json Will follow up a new PR to unwire completely the .jsons once this is complete opendatahub-io/kubeflow#513

@atheo89 , removal of runtime has been done at line 106: 8745417#diff-bf1b39ab6e296ed2806e2d5b1c26436ded10fb59f70b1e94334942808596193dL106
so the workbench image won't have runtime image anymore.
Screenshot from 2025-03-05 16-23-23

@atheo89
Copy link
Member Author

atheo89 commented Mar 10, 2025

Thanks @harshad16 as agreed, I opened this PR to fix this: #944

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