-
Notifications
You must be signed in to change notification settings - Fork 140
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
support python 3.12 #619
support python 3.12 #619
Conversation
Signed-off-by: Alexey Roytman <[email protected]>
Signed-off-by: Alexey Roytman <[email protected]>
Signed-off-by: Alexey Roytman <[email protected]>
.make.defaults
Outdated
@@ -51,7 +51,7 @@ DOCKER_LOCAL_IMAGE=$(DOCKER_IMAGE_NAME):$(DOCKER_IMAGE_VERSION) | |||
DOCKER_REMOTE_IMAGE=$(DOCKER_REGISTRY_ENDPOINT)/$(DOCKER_IMAGE_NAME):$(DOCKER_IMAGE_VERSION) | |||
DOCKER_SPARK_BASE_IMAGE_NAME=data-prep-kit-spark-3.5.2 | |||
DOCKER_SPARK_BASE_IMAGE=$(DOCKER_SPARK_BASE_IMAGE_NAME):$(DOCKER_IMAGE_VERSION) | |||
RAY_BASE_IMAGE?=docker.io/rayproject/ray:${RAY}-py310 | |||
RAY_BASE_IMAGE?=docker.io/rayproject/ray:${RAY}-py312 |
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.
Can we make Python version a separate parameter in make.versions? I am sure it can be used elsewhere
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.
We can, but this format (py312) is used only here, so I don't think we should propagate it into make.versions
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.
Also this is kinda dangerous. what if I am on Python3.10? It really should get current Python version and adjust the base Ray image accordingly
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.
No, here the question is the Python version of the images we deploy to the repository. This includes the RayKFP image. It doesn't connect to the Python version of a certain user.
The original issue is to support Python v3.12, now we do support it. Should we change our Python default version to v3.12? probably not.
I'm going to return it to v3.10, but I'm open to other ideas.
CC: @daw3rd , @shahrokhDaijavad @yuanchi2807 @Bytes-Explorer @blublinsky
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.
Aren't images different than venv? Base images can be whatever works, as long as we do the transform tests in the image, which we are.
Separately, we probably need to start using a github workflow matrix to test at least 3.11 and 3.12. This requires changing .github/workflow/test-transform.template somehow and then regenerating them with make
.
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.
Ray is very sensitive to its and Python versions; they should be the same on clients and servers. We should provide a set of images, or users will have to build the images by themselves.
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.
OK, I validated that all tests passed with Python 3.12, not I will return the default setting to 3.10.
Why exactly we need python 3.12 |
During Sujee's workshop, he was asked several times about Python 3.12 |
you are checking the |
Sorry, my bad |
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 slack me when ready to merge. Thanks
Hi @touma-I , please take a look |
@@ -118,9 +118,11 @@ KFP_v2=2.2.0 | |||
KFP_v2_SDK=2.8.0 | |||
KFP_v1=1.8.5 | |||
KFP_v1_SDK=1.8.22 | |||
RAY=2.24.0 | |||
RAY=2.36.1 |
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.
@roytman just to confirm: Ray 2.36.1 will work with Python 3.10, 3.11 and 3.12. right?
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 ran all CI/CD tests with Python 3.12 and now it runs with Python 3.10. So yes, it works.
Why are these changes needed?
There is a user requirement to support Python 3.12.
Ray started to support Python 3.12 from
2.36.0
. This PR upgrades Ray to2.36.1
as it's bug fix release.Related issue number (if any).
#618