-
Notifications
You must be signed in to change notification settings - Fork 375
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
Use docker to pin dependencies #829
Conversation
7464108
to
f4393b5
Compare
python-version: [ '3.10' ] | ||
cirq-version: [ '~=1.0' ] | ||
# Also check least-supported versions (linux only) | ||
include: |
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's the minimum python version we support now?
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.
Instead of abusing the strategy matrix, I moved the "least supported" test to a new job pytest-minimum
. There, I bumped the minimum python version for shady reasons:
- The "minimum" environment uses cirq 0.15, which supports python 3.8 so we could run the job on python 3.8
- however, the way the job works is first you install the full, modern, pinned environment which requires python 3.9 because cirq 1.2 requires python 3.9
- After the full environment, we install the older version of cirq. But that step would fail with the old python
The "correct" fix would be to pin a minimal environment. The correct way to do this would be to have another Dockerfile that uses python 3.8
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 have unbumped the minimum python version by doing a correct fix
SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" &>/dev/null && pwd)" | ||
readonly SCRIPT_DIR | ||
|
||
# We use docker to run the actual `pip-compile` command because its |
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 is the workflow for regenerating new requirement files now? Should we run this script on our local machine and add?
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.
If you run re-pip-compile-in-docker.sh
on a workstation with docker installed, it should just work ™️
You can also run the python script outside of docker but its output may vary depending on the environment you run it from
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.
More of a discussion question, but it looks like we're moving to only testing python3.10, which seems fine, should we add 3.9 / 3.11?
But yeah, this is generally a much simpler way of managing environments. |
a4f21a5
to
91c88ec
Compare
ok, for better or for worse I have changed my pip-compile driver script to be able to support multiple "platforms" where each platform is a collection of environment (pinned requirement) files. Within a platform they're all consistent. So here: the default furthermore: the python version of the docker image is now parameterized so we generate the |
@mpharrigan is this good to go? |
Sorry, I'll have to revisit this and give it the once-over to make sure it's good to go |
Assuming the macos runners are down for the moment. |
No description provided.