-
Notifications
You must be signed in to change notification settings - Fork 63
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
Update runtime's python packages #706
Update runtime's python packages #706
Conversation
@@ -1,24 +1,24 @@ | |||
# Copied from: https://github.com/elyra-ai/elyra/blob/main/etc/generic/requirements-elyra.txt | |||
|
|||
# This is a comprehensive list of python dependencies that Elyra requires to execute Jupyter notebooks. | |||
ipykernel==6.13.0 | |||
ipython==8.10.0 | |||
ipykernel==6.29.5 |
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.
@harshad16 I've manually updated this file to the latest versions, and I've applied the same versions to the Pipfiles example as well.
The packages on the requirments.txt on elyra repo looks old: https://github.com/elyra-ai/elyra/blob/main/etc/generic/requirements-elyra.txt
Since I wasn't certain which versions to use, I opted to go with the latest updates.
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.
Personally I would remove any changes to these requirements-elyra.txt
files from this PR. Or delete them completely to comply with: https://issues.redhat.com/browse/RHOAIENG-11068.
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.
All the changes look good.
as we are not using the requirements-elyra.txt, we would anyway use the https://issues.redhat.com/browse/RHOAIENG-11068, to de-clutter this.
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.
Tnx for pointing this out, the changes have been incorporated under this commit 664ea37
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.
everything looks good
just one note
391b967
to
56edd33
Compare
scipy = "~=1.14.1" | ||
skl2onnx = "~=1.17.0" | ||
onnxconverter-common = "~=1.13.0" # Required for skl2onnx, as upgraded version is not compatible with protobuf | ||
codeflare-sdk = "~=0.18.0" |
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.
Is this version decrease intentional? 🤔 (same applies for this change in the rest of the files)
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.
Yeah, because on jupyter ds notebook we have onnxconverter-common = "~=1.13.0"
I kept the same on the runtime as well
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 understand the reason for the onnxconverter-common - but my question was for the codeflare-sdk.
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 looked here - so I guess you tried to aligned these images... well, it's probably not a big deal for now as the regular codeflare-sdk update should fix it for all the images. But we should be sure we'll run it explicitly before the release then...
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.
Please separate the change of removing requirements.txt , out of this PR
for smooth moment of this work.
664ea37
As de-clutter of requirements.txt needs a little bit more work, which should be addressed in a separate PR. The reason being, till we have this env var set here:
export ELYRA_REQUIREMENTS_URL="file:///opt/app-root/bin/utils/requirements-elyra.txt" |
Best to separate it out in a different PR, to be address that.
664ea37
to
56edd33
Compare
Good point, here you may find the draft PR for this specific workload #708 (The commit has been removed from the current one) |
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.
/lgtm
/approve
Thanks 💯
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: harshad16 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 |
/override ci/prow/images |
@harshad16: Overrode contexts on behalf of harshad16: ci/prow/images, ci/prow/rocm-runtimes-ubi9-e2e-tests, ci/prow/runtime-rocm-pytorch-ubi9-python-3-11-pr-image-mirror, ci/prow/runtime-rocm-tensorflow-ubi9-python-3-11-pr-image-mirror, ci/prow/runtimes-ubi9-e2e-tests In response to this:
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. |
93f441c
into
opendatahub-io:main
Since this is merged with the codeflare-sdk 0.18.0 - do we plan to update it to the latest version before the release? |
Exactly, there is a tracker for this, once all the related issues are done we could proceed on the upgrade https://issues.redhat.com/browse/RHOAIENG-12507 |
Related to: https://issues.redhat.com/browse/RHOAIENG-13121
Description
How Has This Been Tested?
Merge criteria: