pennylane icon indicating copy to clipboard operation
pennylane copied to clipboard

Add build-mlir rule to docker/Makefile.

Open erick-alt-delete opened this issue 2 years ago • 5 comments

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 to compile-image in the llvm.dockerfile. I noticed that the docker COPY command is dependent on the PATH to the docker 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 this data-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.

erick-alt-delete avatar Apr 14 '22 09:04 erick-alt-delete

Thanks for the submission @erick-alt-delete . We will take a look and can start a discussion during the review.

mlxd avatar Apr 14 '22 12:04 mlxd

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.

codecov[bot] avatar Apr 14 '22 14:04 codecov[bot]

I will make the requested changes, rebase, squash and force push later today or tomorrow. Thanks for the quick review!

erick-alt-delete avatar Apr 14 '22 16:04 erick-alt-delete

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.

mlxd avatar Apr 14 '22 16:04 mlxd

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 or interface doesn't capture our needs, but I'll leave others comment if that needs updating.

mlxd avatar Apr 14 '22 17:04 mlxd

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).

trbromley avatar Nov 24 '23 13:11 trbromley

Thanks everyone! Closing this because it's no longer needed.

trbromley avatar Nov 28 '23 14:11 trbromley