-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/nix support #120
base: main
Are you sure you want to change the base?
Feature/nix support #120
Conversation
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.
Thank you for the suggestion!
I am not familiar with nix, so I have added a few questions / comments
.github/workflows/build_with_nix.yml
Outdated
@@ -0,0 +1,20 @@ | |||
name: build with nix | |||
on: | |||
schedule: |
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 you give some details why we would not want to run this on PRs? In case of changes we would like to know that this fails?
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.
Ah, the moment we don't have a Cachix subscription, so we sint have a central store to place things that we have built before. This includes tket - so when this CI runs, it will need to build tket first.
Once we have a caching subscription, the latest tket build will already be available, so building and testing pytket-qir will just be a case of bundling the python package and running checks on it. Then, if we store the result in cachix, anyone can load up a pre-built environment with tket, pytket and pytket-qir very quickly.
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.
Thank you for all the details!
flake.lock
Outdated
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.
Is this a file we want to have in the git?
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. The flake.lock pins the nix flake dependencies to well-defined versions, which helps keep the repository and users of the repository in sync.
It's feasible that a future commit to nixpkgs e.g. breaks mypy, or updates scipy with a breaking change. By keeping the flake.lock file in the repository, maintainers of this repository get to decide when nixpkgs, tket etc can get updated, by pushing changes created by invoking nix flake update
.
llvm = super.llvm_14; | ||
llvm-v-major = lib.versions.major llvm.version; | ||
llvm-v-minor = builtins.substring 0 1 (lib.versions.minor llvm.version); | ||
pyqir-version = "0.10.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.
Is it possible to import the version from the python packages here, or do we now need to update this in two places?
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.
In this case it is customary to provide the exact git revision to pull, then specify the build process, as pyqir isn't available on nixpkgs.
I can update it to scan setup.py for a specific version, though if it changes to using a loose version match then the process will fail.
Though I will add that I am committed to supporting the nix support for this repository and others, so if a change breaks the nix support, or if a version update is required, I'm absolutely happy to action it.
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 case I am doing the update, is there anything else I need to do besides updating the version in this file as well?
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.
Do you have plans to add nix for the other extensions as well?
.github/workflows/build_with_nix.yml
Outdated
@@ -0,0 +1,20 @@ | |||
name: build with nix | |||
on: | |||
schedule: |
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.
Thank you for all the details!
llvm = super.llvm_14; | ||
llvm-v-major = lib.versions.major llvm.version; | ||
llvm-v-minor = builtins.substring 0 1 (lib.versions.minor llvm.version); | ||
pyqir-version = "0.10.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.
I case I am doing the update, is there anything else I need to do besides updating the version in this file as well?
And it would be nice if you could add something to the changelog? |
For some reason my github app isnt letting my reply in thread, so:
If the new version requires to change to the setup process, then this is my usual update process:
The nix build log will then emit a warning along the lines of: Once it has built the package it will calculate the actual hash. It will emit an error along the lines of:
What I do then is add the provided hash back into the nix file and run nix build again. It should immediately notice that it has already seen a derivation with this source, build instructions, and output hash, and resolve to the package it previously built. The hash thing is annoying but extremely useful. If Nix can verify the hash then it knows if it has seen it before and lets it skip steps. Likewise, subsequent runs on that machine will serve that result immediately. If we get a cachix subscription, we can upload the resulting derivation there, and any user that needs it (as specified by the matching source, build instructions and output hash) will be offered a ready-to-go downloadable package instead of going through the build and test process.
Indeed! I was asked to look at pytket-quantinuum and pytket-qir is a dependency of it (in the testing pipeline) so I started here. This, with the above about the caching possibilities, is quite exciting. Cachix is when the real benefit of the nix integration will kick in: users will be able to run It doesn't stop at environments, but can also launch tools. So we could possibly have a one-liner to launch a Jupiter notebook pre-filled with demo code, with the environment all loaded up, ready to showcase. But this is all future ifs and maybes, and I don't want to push it in a direction you don't want to take it in. |
Thank you for all the information! I think it would be helpful if you could just copy this into a readme file and add this to the repo as well. Just to make sure this is documented somewhere. Happy to review your PR on pytket-quantinuum as well. |
Maybe it would be good if we could add this to pytket-qiskit as well? As they are both the most used extensions? |
That is my next challenge after I've got pytket-quantinuum in.
There are qiskit libraries already available on the nix store, however they
are marked as broken due to conflicts with symengine. So the qiskit
integration PR might be a bit more substantial of a PR than this one,
depending on how it goes.
…On Tue, 30 Jan 2024, 14:59 cqc-melf, ***@***.***> wrote:
Maybe it would be good if we could add this to pytket-qiskit as well? As
they are both the most used extensions?
—
Reply to this email directly, view it on GitHub
<#120 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/APSUR26F3C7Q6DHU2OZTNMDYREDDTAVCNFSM6AAAAABCPZQNWKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJXGA3TQNRRGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
@jake-arkinstall is this ready for re-review? |
Yes, it is! Although as some movement looks to have been made on the cachix side it might be worth waiting for that to land in tket itself, as it'll speed things up considerably (this repo can then rely on the tket's cached outputs instead of forcing a build). |
Please rebase and retarget to |
Added a nix flake and derivations for pyqir and pytket-qir, using the tket flake from github for pytket module inclusion. The tket flake's nixpkgs is overridden with the current package's flake to avoid unnecessary download, and to help spot compatibility problems more easily. Currently mypy tests are disabled because there are several type errors that appear. I do not know if these are expected, or if it is a problem with this derivation. I have also tested with my `feature/nix-support-add-mypy` branch of tket and, although the errors are reduced, there are some outstanding mypy issues coming from within this repository (at least, on the nix build). Advice would be appreciated on this aspect. Added result directory to .gitignore, as this can be created by nix during development.
Previous mypy failures were due to scipy and IPython stubs missing. By adding them to mypy.ini and setting them to ignore, mypy checks now work.
c34ceb2
to
ab0a4c2
Compare
I have rebased to For the workflow to succeed, CACHIX_AUTH_TOKEN will need to be available in the workflow environment, as in the tket repository. I have also run |
(Additionally, I set nixpkgs to follow TKET's - this will help with possible cache misses if the two disagree on nixpkgs. As more weight is on TKET's build, doing it this way around seems to make the most sense) |
I added |
Closing and reopening to trigger CI... |
Description
Added nix support, The nix flake fetches the pytket flake to fetch pytket, and uses a custom derivation to generate pyqir. mypy and pytest are run as part of
nix flake check
, which is performed in CI (currently set to every sunday at 2am).At the moment, tket and pytket will be built. The speed of performing the flake check will be improved significantly if we use cachix.
To try out this flake:
Then invoke python.
pytket.qir
is importable and working, e.g.:After the merge of this PR, the development shell will be accessible with
nix develop github:CQCL/pytket-qir
Checklist