-
Notifications
You must be signed in to change notification settings - Fork 69
chore: upgrade to python 3.12 for crud-web-apps #757
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
chore: upgrade to python 3.12 for crud-web-apps #757
Conversation
| "kubernetes >= 22.6.0", | ||
| "requests >= 2.32.4", | ||
| "urllib3 >= 2.5.0", | ||
| "Werkzeug >= 3.0.6", |
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.
The current python_requires=">=3.6" is incompatible with the updated dependencies. I checked the actual Python requirements on PyPI:
Dependencies that require Python ≥3.9:
urllib3 >= 2.5.0→ requires Python ≥3.9gevent(latest) → requires Python ≥3.9
Dependencies that support Python ≥3.8:
Flask >= 2.3.2→ requires Python ≥3.8Werkzeug >= 3.0.6→ requires Python ≥3.8requests >= 2.32.4→ requires Python ≥3.8kubernetes >= 22.6.0→ requires Python ≥3.6
Recommendation:
Since this PR was tested with Python 3.12 and some dependencies require ≥3.9, I suggest:
python_requires=">=3.12", # Tested version, most conservative Or at minimum:
python_requires=">=3.9", # Minimum required by urllib3 2.5.0 and gevent (that is the permissive suggestion).
Also consider removing line 13:
"importlib-metadata >= 1.0;python_version<'3.8'",
This condition will never be true when python_requires is ≥3.9, making it redundant.
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.
Great observation, totally agreed and modified, thanks.
d0ab7b8 to
54c8fa6
Compare
|
/lgtm |
andyatmiami
left a comment
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.
Thanks @asaadbalum - this is looking really good - just a little more cleanup I'm suggesting here.
In addition to included review comments - please also be mindful of changes needed to this file:
We should make sure that we are referring to 3.12 in the example Docker snippet.
I also think we should clean up some deprecated code as part of this work too. The following lines should use log.warning (as opposed to the deprecated log.warn)
- https://github.com/kubeflow/notebooks/blob/notebooks-v1/components/crud-web-apps/common/backend/kubeflow/kubeflow/crud_backend/config.py#L62
- https://github.com/kubeflow/notebooks/blob/notebooks-v1/components/crud-web-apps/common/backend/kubeflow/kubeflow/crud_backend/__init__.py#L26
We can see log.warning already being employed in the codebase - including - but not limited to - here:
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 THINK the following file is unused.. and given the changes here to setup.py - is now a source of outdated (mis)information:
We should remove this requirements.txt file (assuming its removal doesn't break anything!)
.github/workflows/python_lint.yaml
Outdated
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Python environment 3.8 | ||
| - name: Set up Python environment 3.12 |
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.
| - name: Set up Python environment 3.12 | |
| - name: Set up Python |
shorter name == better name 😎
I want to get out of specifying the version in the name to reduce number of lines that would ever need changes on future update..
|
/ok-to-test |
Upgrade Python toolchain from 3.10 to 3.12 for all three CRUD web applications (Jupyter, Volumes, Tensorboards) and their shared common backend. Changes: - Update Dockerfiles to use python:3.12-slim base image - Add setuptools and wheel installation (required in Python 3.12 slim) - Update CI workflows to use Python 3.12 for testing and linting - Upgrade common backend dependencies to address vulnerabilities: * Flask: 1.1.1 → 2.3.2 * Werkzeug: 0.16.0 → 3.0.6 * requests: 2.22.0 → 2.32.4 * urllib3: 1.25.7 → 2.5.0 * kubernetes: ==22.6.0 → >=22.6.0 Testing performed: - All CI workflows passed (backend unit tests, integration tests, multi-arch builds) - Local functional testing in Kind cluster with full Kubeflow deployment - Verified all three web apps running on Python 3.12 - Tested CRUD operations via UI: * Created, viewed, and deleted Volumes * Created, viewed, and deleted Jupyter Notebooks * Created, viewed, and deleted TensorBoards - Verified namespace visibility and RBAC permissions - Confirmed API endpoints responding correctly - Validated container startup and health checks Closes: kubeflow#724, kubeflow#725, kubeflow#726 Signed-off-by: Asaad Balum <[email protected]>
54c8fa6 to
face97b
Compare
Signed-off-by: Mathew Wicks <[email protected]>
Signed-off-by: Mathew Wicks <[email protected]>
|
We must pin the version of the Once I did this, it became clear that we were not using the version of |
|
Whenever we cut a release, we can do more thorough testing, in addition to the testing that's done automatically in the manifest repo, but I imagine anything that breaks would be pretty obvious. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: thesuperzapper 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 |
0b835a7
into
kubeflow:notebooks-v1
Description
Upgrades Python toolchain from 3.10 to 3.12 for all three CRUD web applications (Jupyter, Volumes, Tensorboards) and their shared common backend.
Closes #724, #725, #726
Changes Made
Dockerfiles
python:3.12-slimfor all three web appssetuptoolsandwheelinstallation (required in Python 3.12 slim images)components/crud-web-apps/jupyter/Dockerfilecomponents/crud-web-apps/volumes/Dockerfilecomponents/crud-web-apps/tensorboards/DockerfileCI Workflows
.github/workflows/jwa_backend_unittests.yaml.github/workflows/python_lint.yamlDependencies
Updated
components/crud-web-apps/common/backend/setup.pyto address security vulnerabilities:1.1.1→≥2.3.20.16.0→≥3.0.62.22.0→≥2.32.41.25.7→≥2.5.0==22.6.0→≥22.6.0Documentation
Testing Performed
✅ CI Workflows (All Passed)
✅ Local Functional Testing
Tested in Kind cluster with full Kubeflow deployment:
Infrastructure:
CRUD Operations Verified:
Additional Verification:
Acceptance Criteria
Notes
setuptoolsandwheelby default, so explicit installation was added≥) for better flexibilitykubernetespackage constraint was relaxed from==22.6.0to≥22.6.0to allow compatible updates