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

Allow lightning.qubit to skip C++ compilation and provide a Python-only binary #129

Merged
merged 23 commits into from
Aug 13, 2021

Conversation

trbromley
Copy link
Contributor

@trbromley trbromley commented Aug 11, 2021

Context:

PennyLane-Lightning can now be installed without compiling its C++ binaries and will fall back
to using the default.qubit implementation. Skipping compilation is achieved by setting the
SKIP_COMPILATION environment variable, e.g., export SKIP_COMPILATION=True. This feature
is intended for building a pure-Python wheel of PennyLane-Lightning as a backup for platforms
without a dedicated wheel.

Possible Drawbacks:

Users installing the non-compiled version will obviously have a device that is as fast as default.qubit. A warning has been added for this eventuality.

@@ -116,7 +116,7 @@ def build_extensions(self):
build_ext.build_extensions(self)


if not os.environ.get("MOCK_DOCS", False):
if not os.environ.get("SKIP_COMPILATION", False):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reminder - I believe this will need the environment variable to be updated in RTD.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, maybe we should add docs testing in the CI tests 🤔 I expected the tests to fail because of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I just added following these instructions.

Copy link
Member

Choose a reason for hiding this comment

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

Building the docs in a PR would be quite useful.

@@ -665,7 +666,7 @@ def circuit(x):
for _ in range(100):
runs.append(circuit(p))

assert np.isclose(np.mean(runs), -np.sin(p), atol=1e-3, rtol=0)
assert np.isclose(np.mean(runs), -np.sin(p), atol=1e-2, rtol=0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was occasionally failing 🤔 So I thought it'd be sufficient to lower the tolerance.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 11, 2021

Test Report (C++) on Ubuntu

    1 files  ±0      1 suites  ±0   0s ⏱️ ±0s
164 tests ±0  164 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
694 runs  ±0  694 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 333ed09. ± Comparison against base commit 333ed09.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Aug 11, 2021

Codecov Report

Merging #129 (bcf65af) into master (e5c980e) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #129   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines           56        71   +15     
=========================================
+ Hits            56        71   +15     
Impacted Files Coverage Δ
pennylane_lightning/lightning_qubit.py 100.00% <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 e5c980e...bcf65af. Read the comment docs.

Comment on lines +61 to +84
build-pure-python-wheel:
runs-on: ubuntu-latest
steps:
- name: Checkout PennyLane-Lightning
uses: actions/checkout@v2
with:
path: main

- uses: actions/setup-python@v2
with:
python-version: '3.7'

- name: Build wheels
run: |
python -m pip install --upgrade pip wheel
cd main
python setup.py bdist_wheel
env:
SKIP_COMPILATION: True

- uses: actions/upload-artifact@v2
with:
name: pure-python-wheels.zip
path: main/dist/*.whl
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main change


upload-wheels:
runs-on: ubuntu-latest
needs: [build-wheels, build-pure-python-wheel]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also added job as a dependency

with:
user: __token__
password: ${{ secrets.PYPI }}
name: Deploy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately the diff for this file is not great, I believe my editor changed from \r\n to \n for the new line style. The main changes are highlighted below.


upload-source:
runs-on: ubuntu-latest
needs: [build-wheels, build-pure-python-wheel]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added as dependency, but not sure it's strictly needed.

@@ -0,0 +1,58 @@
name: Testing without binary
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very similar to the tests.yml workflow but with the c++ tests removed and a few tweaks to test only for without binaries.

@@ -168,7 +168,6 @@ def build_extensions(self):
requirements = [
"numpy",
"pennylane>=0.15",
"pybind11",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note this removal. This means that all the dependencies of lightning are also dependencies of PennyLane.

Copy link
Member

Choose a reason for hiding this comment

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

Just so I'm clear, this change requires the user to ensure they have pybind11 available manually, rather than as an auto-pulled dep when we try to build? As in, explictly running pip install -r requirements.txt is needed?

Copy link
Contributor Author

@trbromley trbromley Aug 12, 2021

Choose a reason for hiding this comment

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

Yes, agreed - users building from source will need to do pip install -r requirements.txt. I added this in the installation instructions: 3c54052

Copy link
Member

@josh146 josh146 Aug 13, 2021

Choose a reason for hiding this comment

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

There might be one additional use-case, not sure how common it is though:

A user wants to pip install lightning on an unsupported system and compile from source.

In this case, the user would have to:

  1. Make sure all C++ dependencies are installed

  2. Run

    pip install pybind11 pennylane-lightning --no-binary :all:
    

Potentially this could occur in Docker workflows. Say a user is using the Alpine Linux image that uses musl; (as far as I am aware?) no wheels are available for this platform, but they

  • would like to avoid using git to reduce the docker image size
  • would like to use the C++ simulator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, have updated the instructions: 8e6ed64

@trbromley trbromley marked this pull request as ready for review August 11, 2021 14:56
@trbromley
Copy link
Contributor Author

[ch8104]

runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [ubuntu-latest, macos-latest, windows-latest]
Copy link
Member

Choose a reason for hiding this comment

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

I wonder can we extract the artifacts create from the other wheel builder scripts, rather than rebuild here. Assuming a successful deploy, we fail to get the new architectures using this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we get the new build scripts (wheel_linux_aarch64.yml, etc) to run also on the published trigger (e.g., like this deploy workflow). We could then have an additional job in each workflow to upload the wheels if the event is published (if: ${{ github.event_name == 'published' }}, taken from here)?

We can then:

  • Take the pure python build into it's own workflow, as you suggested below
  • Reduce this deploy workflow to just uploading the source?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that sounds like a good plan.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! Are you ok if we do that in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, perfectly fine with me.

name: ${{ runner.os }}-wheels.zip
path: ./wheelhouse/*.whl

build-pure-python-wheel:
Copy link
Member

Choose a reason for hiding this comment

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

As with the above comment, I wonder if this should be offloaded into a separate build file, as the case for the other architectures. What do you think?

@@ -116,7 +116,7 @@ def build_extensions(self):
build_ext.build_extensions(self)


if not os.environ.get("MOCK_DOCS", False):
if not os.environ.get("SKIP_COMPILATION", False):
Copy link
Member

Choose a reason for hiding this comment

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

Building the docs in a PR would be quite useful.

.github/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@trevor-vincent trevor-vincent left a comment

Choose a reason for hiding this comment

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

Really good idea Tom, I have nothing to add beyond what Lee has already commented on.

@trbromley trbromley requested a review from mlxd August 13, 2021 11:59
Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

Great job on this @trbromley !

@trbromley trbromley merged commit 333ed09 into master Aug 13, 2021
@trbromley trbromley deleted the provide_universal_binary branch August 13, 2021 15:24
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.

4 participants