-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Always install into a python venv in ci containers #12663
Conversation
8b2922d
to
89f9a53
Compare
Addresses #12608 |
02c0eb7
to
16bff01
Compare
docker/python/freeze-dependencies.sh
Outdated
@@ -1,4 +1,4 @@ | |||
#!/bin/bash | |||
#!/bin/bash -eux | |||
# Licensed to the Apache Software Foundation (ASF) under 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.
oops, rm this file and freeze_deps.py, they are part of a follow-on
|
||
COPY install/ubuntu_install_cmake_source.sh /install/ubuntu_install_cmake_source.sh | ||
# Globally disable pip cache |
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.
# Globally disable pip cache |
@@ -0,0 +1,3 @@ | |||
pip |
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.
why are there separate requirements.txt
s per Python version?
lsb-core | ||
|
||
release=$(lsb_release -sc) | ||
if [ "${release}" == "bionic" ]; 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.
What if we use miniconda instead of the distribution's Python + venv? The end result would be the same but we would be able to say "we use version X of Python in CI" instead of "it depends"
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.
When I was migrating teams off of conda I used pyenv which would compose into our system with Poetry?
The list below shows some tests that ran in main 546a7da but were skipped in the CI build of 5775327:
A detailed report of ran tests is here. |
@driazati i think this is finally ready |
Im hopeful that #12738 will be merged once CI gets green. Happy to merge this just after that. |
@leandron happy to hold off til 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.
LGTM and thanks for working on this!
I'll wait for others to review
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, lets merge this improvements before CI gets stale.
Following change introduced installing python dependencies inside virtual environments: apache#12663 Previous to this fix, a different version of python was being picked up that didn't catch the issues fixed in this commit. Change-Id: Ie290d9474a799311e07d293fa1b8299326b11661
Following change introduced installing python dependencies inside virtual environments: #12663 Previous to this fix, a different version of python was being picked up that didn't catch the issues fixed in this commit. Change-Id: Ie290d9474a799311e07d293fa1b8299326b11661
Recently virtual environments were introduced in the docker images which was a great contribution to localize errors: #12663. In this fix, link to the caffe is created inside this virtual env instead of adding it to the system path of python. This fix also removes importing request package where not needed. Fixes #12663
This PR changes all ci_ to install TVM Python dependencies in a virtualenv separate from the system Python dependencies. Sets the stage for adding the poetry-based dependency generator to the CI container build process. * Always install into a python venv in ci containers. * Respect Dockerfile ENV PATH modifications in docker/bash.sh lookups.
) Following change introduced installing python dependencies inside virtual environments: apache#12663 Previous to this fix, a different version of python was being picked up that didn't catch the issues fixed in this commit. Change-Id: Ie290d9474a799311e07d293fa1b8299326b11661
Recently virtual environments were introduced in the docker images which was a great contribution to localize errors: apache#12663. In this fix, link to the caffe is created inside this virtual env instead of adding it to the system path of python. This fix also removes importing request package where not needed. Fixes apache#12663
This PR changes all ci_ to install TVM Python dependencies in a virtualenv separate from the system Python dependencies. This PR sets the stage for adding the poetry-based dependency generator to the CI container build process.