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

Adds CLI and Python invocations for the device test suite #733

Merged
merged 19 commits into from Aug 4, 2020

Conversation

josh146
Copy link
Member

@josh146 josh146 commented Jul 31, 2020

Context:

  • Currently, the PennyLane device test suite requires that the PennyLane source code be available, or the path to the device tests be known in advance. Further, pytest must be invoked manually.

Description of the Change:

  • Adds a function for finding the location of the installed tests: get_device_tests()

  • Adds a function for invoking the tests from within Python:

    >>> from pennylane.plugins.tests import test_device
    >>> test_device("default.qubit", analytic=True, shots=1024)
    ================================ test session starts =======================================
    platform linux -- Python 3.7.7, pytest-5.4.2, py-1.8.1, pluggy-0.13.1
    rootdir: /home/josh/xanadu/pennylane/pennylane/plugins/tests, inifile: pytest.ini
    plugins: flaky-3.6.1, cov-2.8.1, mock-3.1.0
    collected 86 items
    xanadu/pennylane/pennylane/plugins/tests/test_gates.py ..............................
    ...............................                                                       [ 70%]
    xanadu/pennylane/pennylane/plugins/tests/test_measurements.py .......sss...sss..sss   [ 95%]
    xanadu/pennylane/pennylane/plugins/tests/test_properties.py ....                      [100%]
    ================================= 77 passed, 9 skipped in 0.78s ============================
    >>>
  • Adds a convenient CLI script for invoking the tests, that comes installed with PennyLane:

    $ pl-device-test --device default.qubit --shots 1234 --analytic False --tb=short -x
  • Makes the plugin test suite documentation visible.

  • Small modification to allow passing arbitrary device keyword arguments, using --device-kwargs key1=val1 key2=val2. This flag must come last, as it consumes everything to the right of it.

Benefits:

  • Tests can be easily invoked on external devices even if PennyLane is just pip installed. This will be quite useful for plugins in external repos.

  • Re-uses the existing pytest argument parser in conftest.py to avoid code duplication

Possible Drawbacks:

  • n/a

Related GitHub Issues: n/a

@josh146 josh146 added the review-ready 👌 PRs which are ready for review by someone from the core team. label Jul 31, 2020
@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #733 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #733   +/-   ##
=======================================
  Coverage   95.50%   95.50%           
=======================================
  Files         110      110           
  Lines        6858     6858           
=======================================
  Hits         6550     6550           
  Misses        308      308           
Impacted Files Coverage Δ
pennylane/plugins/__init__.py 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 474982e...27f7efc. Read the comment docs.

@josh146 josh146 requested a review from antalszava July 31, 2020 12:14
@co9olguy co9olguy removed their request for review August 2, 2020 18:44
Copy link
Contributor

@mariaschuld mariaschuld left a comment

Choose a reason for hiding this comment

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

Nice Josh, this makes the test suite a lot more accessible!

Just some comments...

pennylane/plugins/tests/__init__.py Show resolved Hide resolved
pennylane/plugins/tests/pytest.ini Outdated Show resolved Hide resolved

.. code-block:: console

pl-device-test --device default.qubit --shots 1234 --analytic False
Copy link
Contributor

Choose a reason for hiding this comment

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

Took me a while to figure out that one needs to pip install -e . to get this to work...

Strangely, 2 tests for default.qubit fail? Not essential here, but just wondering.

FAILED pennylane/plugins/tests/test_measurements.py::TestSample::test_sample_values_hermitian[device_kwargs0]
FAILED pennylane/plugins/tests/test_measurements.py::TestSample::test_sample_values_hermitian_multi_qubit[device_kwargs0]

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Took me a while to figure out that one needs to pip install -e . to get this to work...

Yep! The setup.py file needs to be run for the CLI script to be placed on the path.

Strangely, 2 tests for default.qubit fail? Not essential here, but just wondering.

These two tests fail stochastically for me; looks like a tolerance issue. Increasing shots to 10000 fixes it :)

pennylane/plugins/tests/__init__.py Show resolved Hide resolved
Comment on lines +120 to +121
>>> from pennylane.plugins.tests import test_device
>>> test_device("default.qubit")
Copy link
Contributor

Choose a reason for hiding this comment

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

Worked great when ran from an ipython session or as part of a Python script! 🙂 💯

In a Jupyter notebook the output was directed into the terminal where jupyter notebook was invoked (and there was no output in the notebook itself).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch about Jupyter notebook! Do you think this is a common use-case we should support? I think we should not support running tests from within a Jupyter notebook

Copy link
Contributor

Choose a reason for hiding this comment

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

Kind of on the fence here 🤔 I think it's not too crucial, though if interactive sessions are supported then a Jupyter notebook would also be an important case. Having said that I think it's not a major point

@@ -209,7 +251,7 @@ def pytest_runtest_makereport(item, call):
# and those using not implemented features
if (
call.excinfo.type == qml.DeviceError
and "not supported on device" in str(call.excinfo.value)
and "supported" in str(call.excinfo.value)
Copy link
Contributor

Choose a reason for hiding this comment

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

How come this needed a change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed that not all of the plugins were standardized in their error messages for unsupported behaviour :( For example, the Cirq plugin raises an 'unsupported' error if you attempt to use certain gates it some situations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see! Hmm, would it be worth adding an extra condition here with or?

Comment on lines +55 to +56
'package_data': {'pennylane': ['plugins/tests/pytest.ini']},
'include_package_data': True
Copy link
Contributor

Choose a reason for hiding this comment

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

If not for this addition, would plugins/tests/pytest.ini be ignored when the test suite is invoked by using the cli?

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue is that the pytest.ini file isn't included in the packaged wheel without this being included! So this forces it to be packaged in the wheel.

By default setuptools will only package up .py files.

Copy link
Contributor

@antalszava antalszava 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! Very nice one @josh146 😊 Awesome that the test suite will be usable for external devices too

@josh146 josh146 merged commit 851ab11 into master Aug 4, 2020
@josh146 josh146 deleted the device-test-cli branch August 4, 2020 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-ready 👌 PRs which are ready for review by someone from the core team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants