Skip to content
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

Merged
merged 8 commits into from
Oct 20, 2023

Conversation

mpharrigan
Copy link
Collaborator

No description provided.

python-version: [ '3.10' ]
cirq-version: [ '~=1.0' ]
# Also check least-supported versions (linux only)
include:
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

Copy link
Collaborator

@fdmalone fdmalone left a 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?

@fdmalone
Copy link
Collaborator

fdmalone commented Aug 7, 2023

But yeah, this is generally a much simpler way of managing environments.

@mpharrigan
Copy link
Collaborator Author

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 envs/ directory has all the normal stuff with the latest cirq whereas max_compat/ envs all have cirq constrained to be 1.0.0

furthermore: the python version of the docker image is now parameterized so we generate the envs/ default envs on python 3.10 and the max_compat/ envs on python 3.8

@fdmalone
Copy link
Collaborator

@mpharrigan is this good to go?

@mpharrigan
Copy link
Collaborator Author

Sorry, I'll have to revisit this and give it the once-over to make sure it's good to go

@mpharrigan mpharrigan marked this pull request as ready for review October 20, 2023 17:58
@fdmalone
Copy link
Collaborator

I need to regenerate as in #843 for black. I'll close #843 in favour of this.

@fdmalone fdmalone self-requested a review October 20, 2023 18:55
@fdmalone fdmalone merged commit 1657dda into quantumlib:master Oct 20, 2023
11 checks passed
@fdmalone
Copy link
Collaborator

Assuming the macos runners are down for the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants