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 pytest-rng generated seeds for stochastic tests #6435

Merged
merged 31 commits into from
Oct 28, 2024
Merged

Conversation

astralcai
Copy link
Contributor

@astralcai astralcai commented Oct 23, 2024

Context:

The PennyLane CI suffers from sporadic failures due to stochasticity of test cases. In order to save CI runtime, we have seeded many tests. However, this approach introduces the risk of a test case passing due to a special seed, potentially hiding bugs that could have been discovered. Therefore, it is beneficial to periodically update the seeds to review such tests.

Description of the Change:

  • The test suite uses pytest-rng to handle seed generation. A seed fixture is now available to use for every test case. If seeding is required, please use the seed provided by pytest-rng
def test_some_function(..., seed):
    dev = qml.device("default.qubit", seed=seed)
  • A local_salt test marker has been added that allows you to modify the seed locally for a particular test case if the generated seed happens to make your test fail.
@pytest.mark.local_salt(42)
def test_some_function(..., seed):
    dev = qml.device("default.qubit", seed=seed)
  • All tests that uses local seeds has been updated to use the new seeding approach.

  • An rng_salt has been added to pytest.ini. This controls the seed generation for the entire test suite. We want to periodically change this rng_salt which updates all the seeds across the test suite.

  • The fixture that sets the global seed for every test case has been removed, and another fixture is added that restores the global seed after each test in case the test modifies the global seed.

Benefits:

We will be able to catch bugs hidden in tests that are passing only because of a magic seed that was set locally.

Possible Drawbacks:

This is a chore to maintain this seed an ensure that all tests pass with the new salt when it is updated.

Related Shortcut Story:

[sc-74294]

Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.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.

@astralcai astralcai requested a review from mudit2812 October 23, 2024 18:11
Copy link

codecov bot commented Oct 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.39%. Comparing base (661dd52) to head (ff4b102).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6435   +/-   ##
=======================================
  Coverage   99.38%   99.39%           
=======================================
  Files         452      452           
  Lines       42789    42787    -2     
=======================================
- Hits        42527    42526    -1     
+ Misses        262      261    -1     

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

tests/pytest.ini Outdated Show resolved Hide resolved
@astralcai astralcai added this to the v0.39 milestone Oct 24, 2024
@astralcai astralcai requested a review from JerryChen97 October 25, 2024 13:55
@astralcai astralcai changed the title Use pytest-rng generated seeds for stochastic tests [WIP] Use pytest-rng generated seeds for stochastic tests Oct 25, 2024
@mudit2812 mudit2812 removed this from the v0.39 milestone Oct 25, 2024
Copy link
Contributor

@mudit2812 mudit2812 left a comment

Choose a reason for hiding this comment

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

Looks good to me. We're still maintaining a "global" seeding behaviour in the sense that we're globally setting a value that affects local seeds. I'm a big fan of this approach, and it's making our tests much more thread safe. I'm happy to approve this PR in its current state, no blocking comments.

tests/conftest.py Show resolved Hide resolved
tests/gradients/core/test_pulse_gradient.py Outdated Show resolved Hide resolved
Copy link
Contributor

@JerryChen97 JerryChen97 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this whole work @astralcai ! It's time for us to get rid of magic power in our previous testings involving any random sampling; I do think we are on the correct track, although probably more perspectives from another senior instead of me should be included.
Also do we have methods to track the traces of salts? Like is it possible to collect the salts for those false negativities.

tests/capture/test_capture_cond.py Show resolved Hide resolved
tests/interfaces/test_jax_jit_qnode.py Show resolved Hide resolved
tests/interfaces/test_jax_jit_qnode.py Show resolved Hide resolved
tests/test_debugging.py Show resolved Hide resolved
tests/conftest.py Show resolved Hide resolved
tests/optimize/test_optimize_shot_adaptive.py Show resolved Hide resolved
tests/test_vqe.py Show resolved Hide resolved
tests/transforms/test_qcut.py Show resolved Hide resolved
@astralcai astralcai enabled auto-merge (squash) October 28, 2024 15:23
@astralcai astralcai merged commit 8dde8f8 into master Oct 28, 2024
40 checks passed
@astralcai astralcai deleted the rng-seeding branch October 28, 2024 16:10
astralcai added a commit that referenced this pull request Oct 30, 2024
KetpuntoG pushed a commit that referenced this pull request Nov 4, 2024
mudit2812 added a commit that referenced this pull request Nov 11, 2024
**Context:**

The PennyLane CI suffers from sporadic failures due to stochasticity of
test cases. In order to save CI runtime, we have seeded many tests.
However, this approach introduces the risk of a test case passing due to
a special seed, potentially hiding bugs that could have been discovered.
Therefore, it is beneficial to periodically update the seeds to review
such tests.

**Description of the Change:**

- The test suite uses `pytest-rng` to handle seed generation. A `seed`
fixture is now available to use for every test case. If seeding is
required, please use the seed provided by `pytest-rng`

```python
def test_some_function(..., seed):
    dev = qml.device("default.qubit", seed=seed)
```

- A `local_salt` test marker has been added that allows you to modify
the seed locally for a particular test case if the generated seed
happens to make your test fail.

```python
@pytest.mark.local_salt(42)
def test_some_function(..., seed):
    dev = qml.device("default.qubit", seed=seed)
```

- All tests that uses local seeds has been updated to use the new
seeding approach.

- An `rng_salt` has been added to `pytest.ini`. This controls the seed
generation for the entire test suite. We want to periodically change
this `rng_salt` which updates all the seeds across the test suite.

- The fixture that sets the global seed for every test case has been
removed, and another fixture is added that restores the global seed
after each test in case the test modifies the global seed.

**Benefits:**

We will be able to catch bugs hidden in tests that are passing only
because of a magic seed that was set locally.

**Possible Drawbacks:**

This is a chore to maintain this seed an ensure that all tests pass with
the new salt when it is updated.

**Related Shortcut Story:**

[sc-74294]

---------

Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
mudit2812 pushed a commit that referenced this pull request Nov 11, 2024
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.

3 participants