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

Solve test measurements issue with CI #895

Merged
merged 15 commits into from
Sep 9, 2024
Merged

Conversation

LuisAlfredoNu
Copy link
Contributor

@LuisAlfredoNu LuisAlfredoNu commented Sep 6, 2024

Before submitting

Please complete the following checklist when submitting a PR:

  • All new features must include a unit test.
    If you've fixed a bug or added code that should be tested, add a test to the
    tests directory!

  • All new functions and code must be clearly commented and documented.
    If you do make documentation changes, make sure that the docs build and
    render correctly by running make docs.

  • Ensure that the test suite passes, by running make test.

  • Add a new entry to the .github/CHANGELOG.md file, summarizing the
    change, and including a link back to the PR.

  • Ensure that code is properly formatted by running make format.

When all the above are checked, delete everything above the dashed
line and fill in the pull request template.


Context:
Revert changes about testing the measurements with shots. The probability of failing the test is non zero then something the test can fail randomly and reduces the CI quality in all PR.

Description of the Change:

Benefits:
Stop failing the CI regarding the measurement test with shots

Possible Drawbacks:

Related GitHub Issues:

@LuisAlfredoNu LuisAlfredoNu marked this pull request as ready for review September 6, 2024 14:29
Copy link
Contributor

github-actions bot commented Sep 6, 2024

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@LuisAlfredoNu LuisAlfredoNu added the ci:build_wheels Activate wheel building. label Sep 6, 2024
Copy link

codecov bot commented Sep 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.17%. Comparing base (7943928) to head (0c9906e).
Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (7943928) and HEAD (0c9906e). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (7943928) HEAD (0c9906e)
9 8
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #895      +/-   ##
==========================================
- Coverage   92.20%   83.17%   -9.03%     
==========================================
  Files          93       24      -69     
  Lines       11376     2146    -9230     
==========================================
- Hits        10489     1785    -8704     
+ Misses        887      361     -526     

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

Copy link
Contributor

@AmintorDusko AmintorDusko left a comment

Choose a reason for hiding this comment

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

Could you please report the change in testing times that this change will bring?

@mlxd
Copy link
Member

mlxd commented Sep 9, 2024

Given the current CI pressure, I am willing to say 1e6 shot workloads aren't something we should be adding to the mix.

@LuisAlfredoNu
Copy link
Contributor Author

@AmintorDusko

Could you please report the change in testing times that this change will bring?

Running locally on a laptop with a CPU: 12th Gen Intel(R) Core(TM) i7-1265U and OMP_NUM_THREADS = 10

Command test:

PL_DEVICE=lightning.qubit python -m pytest tests/lightning_qubit/test_measurements_class.py --maxfail=1

Timing with the current implementation:
2030 passed, 516 skipped in 278.14s (0:04:38)

Timing with master branch
2030 passed, 516 skipped in 253.85s (0:04:13)

Don't change too much

@LuisAlfredoNu
Copy link
Contributor Author

@mlxd I agree with that but the CIs fail randomly and my major concern is to delay the general workflow of the others. That is why I propose this quick fix 😕

I am working on a solution but is not ready yet.

@LuisAlfredoNu
Copy link
Contributor Author

@AmintorDusko

Extracting the run time from the last CI testing in master branch and the current PR

Summary

Test CI master CI current PR
LQ 946 s 941 s
LK CPU 1739 s 1756 s
LK GPU 621 s 616 s
LGPU 133 s 133 s

Run time from CI in master

Python Tests (lightning_qubit, blas-OFF, test-group-1,2,3,4)

=========== 2072 passed, 358 skipped, 3 xpassed in 246.96s (0:04:06) ===========
================ 2070 passed, 363 skipped in 234.28s (0:03:54) =================
=========== 2083 passed, 348 skipped, 2 xfailed in 235.76s (0:03:55) ===========
=========== 2072 passed, 360 skipped, 1 xpassed in 231.76s (0:03:51) ===========

Python Tests (lightning_kokkos, kokkos-4.3.01, model-OPENMP, test-group-1,2,3,4,5,6,7)

= 1084 passed, 351 skipped, 8584 deselected, 2 xfailed, 1 xpassed in 247.77s (0:04:07) =
== 1077 passed, 359 skipped, 8585 deselected, 1 xfailed in 249.85s (0:04:09) ===
== 1079 passed, 356 skipped, 8584 deselected, 3 xpassed in 255.04s (0:04:15) ===
======== 1087 passed, 351 skipped, 8584 deselected in 242.57s (0:04:02) ========
======== 1096 passed, 342 skipped, 8584 deselected in 252.37s (0:04:12) ========
== 1082 passed, 353 skipped, 8584 deselected, 3 xfailed in 243.75s (0:04:03) ===
======== 1091 passed, 346 skipped, 8585 deselected in 251.49s (0:04:11) ========

Python Tests (lightning_kokkos, kokkos-4.3.01, model-CUDA)

= 7352 passed, 2363 skipped, 297 deselected, 6 xfailed, 4 xpassed in 621.49s (0:10:21) =

Python Tests (lightning_gpu, cuda-12)

===== 2887 passed, 1248 skipped, 6 xfailed, 2 xpassed in 133.34s (0:02:13) =====

Run time from CI in current PR

Python Tests (lightning_qubit, blas-OFF, test-group-1,2,3,4)

=========== 2075 passed, 356 skipped, 2 xfailed in 244.50s (0:04:04) ===========
=========== 2078 passed, 355 skipped, 1 xpassed in 227.26s (0:03:47) ===========
================ 2075 passed, 358 skipped in 232.73s (0:03:52) =================
=========== 2070 passed, 360 skipped, 3 xpassed in 238.31s (0:03:58) ===========

Python Tests (lightning_kokkos, kokkos-4.3.01, model-OPENMP, test-group-1,2,3,4,5,6,7)

= 1080 passed, 356 skipped, 8585 deselected, 1 xfailed, 1 xpassed in 256.76s (0:04:16) =
= 1086 passed, 347 skipped, 8586 deselected, 2 xfailed, 2 xpassed in 249.96s (0:04:09) =
======== 1097 passed, 342 skipped, 8584 deselected in 256.74s (0:04:16) ========
======== 1087 passed, 351 skipped, 8585 deselected in 243.43s (0:04:03) ========
======== 1080 passed, 358 skipped, 8585 deselected in 249.57s (0:04:09) ========
= 1079 passed, 356 skipped, 8585 deselected, 2 xfailed, 1 xpassed in 251.63s (0:04:11) =
== 1088 passed, 348 skipped, 8586 deselected, 1 xfailed in 252.41s (0:04:12) ===

Python Tests (lightning_kokkos, kokkos-4.3.01, model-CUDA)

= 7353 passed, 2363 skipped, 297 deselected, 6 xfailed, 4 xpassed in 616.41s (0:10:16) =

Python Tests (lightning_gpu, cuda-12)

===== 2887 passed, 1248 skipped, 6 xfailed, 2 xpassed in 133.13s (0:02:13) =====

@LuisAlfredoNu
Copy link
Contributor Author

@AmintorDusko
The Windows and iOS tests just run the basic penneylane tests suit.

Copy link
Contributor

@AmintorDusko AmintorDusko left a comment

Choose a reason for hiding this comment

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

As times were not quite affected by these changes and precision improved, I'm good with it.
Thank you @LuisAlfredoNu.

Copy link
Member

@maliasadi maliasadi left a comment

Choose a reason for hiding this comment

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

LGTM! Just reverting the meas tests back to their original configs to prevent the random failures on CIs. 👍

@maliasadi maliasadi removed the ci:build_wheels Activate wheel building. label Sep 9, 2024
@LuisAlfredoNu LuisAlfredoNu merged commit a36fe22 into master Sep 9, 2024
71 of 72 checks passed
@LuisAlfredoNu LuisAlfredoNu deleted the testMeasurementsIssue branch September 9, 2024 21:29
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.

5 participants