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

Update CI MacOS to fix notebook test error #6366

Merged
merged 10 commits into from
Dec 2, 2023

Conversation

smburdick
Copy link
Contributor

@smburdick smburdick commented Dec 1, 2023

Description

Address issue #6336 for macOS.

Testing

./check/pytest-changed-files -n auto --ignore=cirq-core/cirq/contrib --enable-slow-tests --verbose

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6ea5ae1) 97.80% compared to head (77ecc83) 97.80%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6366      +/-   ##
==========================================
- Coverage   97.80%   97.80%   -0.01%     
==========================================
  Files        1111     1111              
  Lines       96878    96878              
==========================================
- Hits        94756    94755       -1     
- Misses       2122     2123       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@smburdick smburdick marked this pull request as ready for review December 1, 2023 00:22
@smburdick smburdick requested review from vtomole, cduck and a team as code owners December 1, 2023 00:22
Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but let us check the CI here rather than waiting for the next daily run.

@@ -87,7 +87,7 @@ def env_with_temporary_pip_target():


@pytest.mark.slow
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we temporarily comment out this slow mark to include the test in this PR checks?

If the checks pass, let's put the mark back and merge.

Copy link
Contributor Author

@smburdick smburdick Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what you mean by check the CI here? I thought the PR checks were handling that, but I could be mistaken

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If test is decorated with @pytest.mark.slow it is not included in the CI checks executed for the PR and instead runs in scheduled daily workflows. Removing/commenting the slow mark will run the test in PR checks and we'll know if the fix worked sooner.

@smburdick smburdick force-pushed the notebook-test-fix-macos branch from 7525276 to d292ed6 Compare December 1, 2023 01:43
@smburdick
Copy link
Contributor Author

In failed changed files test:

---------------------------------------------------------------------------
Exception encountered at "In [2]":
---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
Cell In[2], line 5
      2 import networkx as nx
      4 import cirq
----> 5 import quimb
      6 import quimb.tensor as qtn
      7 from cirq.contrib.svg import SVGCircuit

ModuleNotFoundError: No module named 'quimb'

Let's add the quimb module to the requirements (or include Cirq/cirq-core/cirq/contrib/requirements.txt)

@smburdick
Copy link
Contributor Author

The checks passed with the slow mark removed: https://github.com/quantumlib/Cirq/actions/runs/7064136655/job/19231563262?pr=6366

I will restore the slow mark and push.

@smburdick smburdick force-pushed the notebook-test-fix-macos branch from 0ce99be to 4306fcc Compare December 1, 2023 20:12
@smburdick
Copy link
Contributor Author

=========================== short test summary info ============================
FAILED dev_tools/notebooks/notebook_test.py::test_notebooks_against_cirq_head[/home/runner/work/Cirq/Cirq/examples/stabilizer_code.ipynb] - Failed: Notebook failure: stabilizer_code.ipynb, please see out/examples/stabilizer_code.out.ipynb for the output notebook (in Github Actions, you can download it from the workflow artifact 'notebook-outputs')
=================== 1 failed, 55 passed in 502.08s (0:08:22) ===================

I downloaded the artifacts, but don't see that file in the zip.

@smburdick
Copy link
Contributor Author

Retriggered the checks and that problem went away.

@@ -87,7 +87,7 @@ def env_with_temporary_pip_target():


@pytest.mark.slow
@pytest.mark.skipif(sys.platform != "linux", reason="Linux-only test")
@pytest.mark.skipif(sys.platform not in ["linux", "darwin"], reason="Linux/Mac-only test")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@pytest.mark.skipif(sys.platform not in ["linux", "darwin"], reason="Linux/Mac-only test")
@only_on_posix

please reuse the existing decorator from dev_tools.test_utils import only_on_posix
Otherwise LGTM, thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after a small correction. Thank you!

@pavoljuhas pavoljuhas merged commit 30b6c39 into quantumlib:main Dec 2, 2023
35 checks passed
@smburdick smburdick deleted the notebook-test-fix-macos branch December 2, 2023 03:42
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