pennylane
pennylane copied to clipboard
Add build-mlir rule to docker/Makefile.
Allows the creation of a working LLVM/MLIR and Python docker image including pennylane.
- [ ] 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 test directory!
(I don't think all docker images are currently being tested, only base which is failing as seen in https://github.com/PennyLaneAI/pennylane/runs/6018578123?check_suite_focus=true (7370d23beb2f23219156cf37dbfee52e447f46a6). But I'm happy to test it more thoroughly if provided with direction.)
Context: To develop support for MLIR in PennyLane, having a stable working environment will be a requirement. As many pennylane's developers and users work on different systems, having support for all of these can be a challenge. This patch creates a working LLVM/MLIR + Python docker image including PennyLane.
Description of the Change: Adds new recipe for docker image with LLVM/MLIR + Python bindings. Some interesting notes that might come up during review:
- The LLVM version at the moment is hard coded to 14.0.0. This can easily change to an argument but this may introduce the problem that failing to provide a real version the code will crash during building of the docker container and not before. Ideally, the build should fail as soon as possible and not after building pennylane's image.
- A
data-image
in addition tocompile-image
in the llvm.dockerfile. I noticed that the docker COPY command is dependent on the PATH to thedocker build
command. All rules in Makefile set the path to be the current working directory (which probably implies that the Makefile rules are only valid in the root directory). I kept this convention and noticed that it is convention here to use the data in/opt/pennylane
in the pennylane image as needed. I can probably get rid of thisdata-image
if it is a concern. - Not sure if MLIR sould be considered an interface or plugin. Where would it be best organized within the
/docker
folder? - At the moment the only target is X86 but maybe this should also change depending on user input? Happy to discuss this.
- The gpg key lookup is also hard coded.
- Installed llvm/mlir under /opt/llvm.
Tested by using the following:
docker run -it pennylane/mlir bash
python3
> import pennylane as qml
> from mlir.ir import Context, Module
> from mlir.dialects import builtin
Benefits: Stable working environment with LLVM/MLIR + Python bindings.
Possible Drawbacks: Another image to maintain.
Related GitHub Issues: None that I am aware.
Thanks for the submission @erick-alt-delete . We will take a look and can start a discussion during the review.
Codecov Report
Merging #2447 (00f3b96) into master (9f9babb) will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## master #2447 +/- ##
=======================================
Coverage 99.47% 99.47%
=======================================
Files 245 245
Lines 19489 19489
=======================================
Hits 19387 19387
Misses 102 102
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 9f9babb...00f3b96. Read the comment docs.
I will make the requested changes, rebase, squash and force push later today or tomorrow. Thanks for the quick review!
I will make the requested changes, rebase, squash and force push later today or tomorrow. Thanks for the quick review!
Thanks for the work. Once you update based on the above comments, feel free to remove the WIP/Draft and treat this as a PR ready for review. I'll tag some colleagues to also get their eyes on reviewing this too. Also, tomorrow is a national holiday here is Toronto, so expect a response on Monday at the earliest.
Also, to address your earlier questions:
- Since the docker builds are substantial in time, we tend not to test on each PR. I think this is also sensible here as LLVM builds are quite heavy on CI systems.
- As you say, the makefile should always be run from the root repo directory, so all good.
- The current structure of where you placed the files I think is fine for our needs. Since the interface focuses on ML frameworks, we can collectively decide if this needs to move later. I think
plugin
orinterface
doesn't capture our needs, but I'll leave others comment if that needs updating.
Hi @erick-alt-delete! What is the status of this PR? Given that the PL docker support is not that well maintained, I don't think it's a big priority to add this. Note that we have separate docker builds for the lightning ecosystem (@vincentmr).
Thanks everyone! Closing this because it's no longer needed.