fix: install vendored python 3.11 on ubuntu focal for analytics api #278
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the analytics API role to install Python 3.11 from vendored packages on Ubuntu Focal instead of using the standard repository installation. This is necessary because Ubuntu Focal (20.04) is out of support and Python 3.11 is not available in its default repositories.
Key changes:
- Switches from system Python 3.11 installation to vendored package installation controlled by a feature flag
- Adds tasks to download and install Python 3.11 packages from a vendored GitHub repository
- Disables the default Python 3.11 installation in the edx_django_service dependency
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| playbooks/roles/analytics_api/tasks/main.yml | Adds new tasks to conditionally install either standard or vendored Python 3.11 packages |
| playbooks/roles/analytics_api/meta/main.yml | Disables Python 3.11 installation in the edx_django_service role dependency |
| playbooks/roles/analytics_api/defaults/main.yml | Defines configuration variables for vendored Python 3.11 package installation including URLs, build versions, and dependencies |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # | ||
|
|
||
| - name: install python3.11 | ||
| when: not use_vendored_py311 |
There was a problem hiding this comment.
The condition logic is inverted. When use_vendored_py311 is true, the standard Python 3.11 installation task should be skipped, but this condition will execute the standard installation when vendored packages are being used. Change to when: not use_vendored_py311 | default(false) or verify the intended logic.
| when: not use_vendored_py311 | |
| when: not use_vendored_py311 | default(false) |
| # Build string that's present in the deb file names. (Just used to make the | ||
| # names below easier to read.) | ||
| vendored_py311_build: "3.11.13-15-g8adac492d4-1+focal1" | ||
| # These must be kept topologically sorted, dependant packages last, as they |
There was a problem hiding this comment.
Corrected spelling of 'dependant' to 'dependent'.
| # These must be kept topologically sorted, dependant packages last, as they | |
| # These must be kept topologically sorted, dependent packages last, as they |
|
|
||
| - name: "Install vendored Python 3.11 packages" | ||
| when: use_vendored_py311 | ||
| command: dpkg -i "/tmp/vendored_python_{{ vendored_py311_build }}/{{ item }}" |
There was a problem hiding this comment.
Using command module with dpkg -i will always report as 'changed' even if packages are already installed. Consider using the apt module with deb parameter or add creates parameter to make the task idempotent and prevent unnecessary reinstallations on subsequent runs.
| command: dpkg -i "/tmp/vendored_python_{{ vendored_py311_build }}/{{ item }}" | |
| apt: | |
| deb: "/tmp/vendored_python_{{ vendored_py311_build }}/{{ item }}" |
| dependencies: | ||
| - role: edx_django_service | ||
| edx_django_service_use_python311: true | ||
| edx_django_service_use_python311: false |
There was a problem hiding this comment.
If the edx_django_service role is what's installing Python 3.11 in the first place, you'll probably need to make these changes there instead of in the analytics_api role.
There was a problem hiding this comment.
OK yeah, looking further at edx_django_service, the changes definitely need to be there instead -- it then proceeds to make a venv, install Python dependencies, etc.
There was a problem hiding this comment.
If the
edx_django_servicerole is what's installing Python 3.11 in the first place, you'll probably need to make these changes there instead of in theanalytics_apirole.
edx_django_service role is being used by almost all the roles, is it safe to make the changes there?
There was a problem hiding this comment.
Yes, if you add a similar use_vendored_py311 toggle. But also remember that a lot of those roles are no longer used -- most services have been switched to Docker and Kubernetes.
There was a problem hiding this comment.
@timmc-edx In edx_django_service, the edx_django_service_use_python311 flag is only used in the two tasks pasted below. I think if we copy/paste these tasks into analytics_api, then we are good to go. Please share your thoughts. I have also updated the PR.
- name: install python3.11
apt:
pkg:
- python3.11-dev
- python3.11-distutils
update_cache: yes
register: install_pkgs
until: install_pkgs is success
retries: 10
delay: 5
when: edx_django_service_use_python311 and not edx_django_service_enable_experimental_docker_shim
tags:
- install
- install:system-requirements
- name: build virtualenv with python3.11
command: "virtualenv --python=python3.11 {{ edx_django_service_venv_dir }}"
args:
creates: "{{ edx_django_service_venv_dir }}/bin/pip"
become_user: "{{ edx_django_service_user }}"
when: edx_django_service_use_python311 and not edx_django_service_enable_experimental_docker_shim
tags:
- install
- install:system-requirementsThere was a problem hiding this comment.
I don't think that will work, for the reasons I mentioned above.
muhammad-ammar
force-pushed
the
ammar/use-focal-py
branch
2 times, most recently
from
7bf1a02 to
6317524
Compare
muhammad-ammar
force-pushed
the
ammar/use-focal-py
branch
3 times, most recently
from
5ab33b5 to
6f068ff
Compare
|
@timmc-edx Please review now. I have removed the changes from the analytics_api role and moved them inside |
muhammad-ammar
force-pushed
the
ammar/use-focal-py
branch
from
6f068ff to
d96153a
Compare
|
@timmc-edx Please review the new changes. |
2 participants
Description: Changes in this PR have been copied from #269, #270 and #272
Make sure that the following steps are done before merging: