-
Notifications
You must be signed in to change notification settings - Fork 41
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
Conversation
@@ -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): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
Codecov Report
@@ Coverage Diff @@
## master #129 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 56 71 +15
=========================================
+ Hits 56 71 +15
Continue to review full report at Codecov.
|
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 |
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
-
Make sure all C++ dependencies are installed
-
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
There was a problem hiding this comment.
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
[ch8104] |
runs-on: ${{ matrix.os }} | ||
strategy: | ||
matrix: | ||
os: [ubuntu-latest, macos-latest, windows-latest] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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.
Co-authored-by: Lee James O'Riordan <[email protected]>
There was a problem hiding this 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.
There was a problem hiding this 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 !
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 theSKIP_COMPILATION
environment variable, e.g.,export SKIP_COMPILATION=True
. This featureis 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.