Prevent notebook tests from changing the pytest Python environment #6032
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
test_notebooks_against_released_cirq should not change the installed packages in the Python pytest environment. Here we expose downgrade of ply to ply==3.4 in the test of interop.ipynb.
Execute papermill and notebook tests in an OS environment that tells pip to install new packages to a temporary directory. `pip install` commands in the notebook should thus not change the environment that runs pytest.
pavoljuhas
added
the
kind/health
pavoljuhas
added a commit
to pavoljuhas/Cirq
that referenced
this pull request
Mar 13, 2023
Keep users safe from downgrading their ply package installed with cirq. Related to quantumlib#6032
viathor
approved these changes
Mar 23, 2023
Collaborator
viathor
left a comment
viathor
left a comment
There was a problem hiding this comment.
LGTM with a minor comment and a question to confirm the intended behavior.
dev_tools/notebooks/notebook_test.py
Outdated
| packages_before = set((d.name, d.version) for d in importlib.metadata.distributions()) | ||
| yield | ||
| packages_after = set((d.name, d.version) for d in importlib.metadata.distributions()) | ||
| packages_changed = packages_before.difference(packages_after) |
Collaborator
There was a problem hiding this comment.
Shorter and more readable:
assert packages_before.issubset(packages_after)or
assert packages_before == packages_afterfor symmetric difference (see other comment).
Collaborator
Author
There was a problem hiding this comment.
Thanks for catching this, see the fix below.
dev_tools/notebooks/notebook_test.py
Outdated
| packages_before = set((d.name, d.version) for d in importlib.metadata.distributions()) | ||
| yield | ||
| packages_after = set((d.name, d.version) for d in importlib.metadata.distributions()) | ||
| packages_changed = packages_before.difference(packages_after) |
Collaborator
There was a problem hiding this comment.
Don't we actually want the symmetric difference here? IOW, don't we also want to prevent tests from changing the environment by installing new packages?
Also fail if test_notebooks_against_released_cirq adds any new package. Executions of `pip install` should deploy to a temporary directory.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
test_notebooks_against_released_cirqshould not change the installedpackages in the Python pytest environment. Adjust OS environment
so that pip invoked from tested notebooks installs to a temporary directory.
Also add test for changed packages that catches downgrade to ply==3.4
caused by a test of interop.ipynb.