feat: Move the compiled eccs to a separate package #517

aborgna-q · 2024-07-30T16:49:16Z

Closes #486. We should merge this before publishing the next alpha.

I checked that the generated wheels for tket2-py contain the correct (non-local) dependency on tket2_eccs.

The pure-python wheels ci job is copied from hugr-py.

Release-As: 0.1.0

codecov · 2024-07-30T16:56:23Z

Codecov Report

Attention: Patch coverage is 55.55556% with 8 lines in your changes missing coverage. Please review.

Project coverage is 83.96%. Comparing base (3e67379) to head (81b3e7e).
Report is 1 commits behind head on main.

Files	Patch %	Lines
tket2-py/tket2/rewrite.py	0.00%	6 Missing ⚠️
tket2-py/tket2/passes.py	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #517      +/-   ##
==========================================
+ Coverage   83.64%   83.96%   +0.32%     
==========================================
  Files          60       61       +1     
  Lines        6572     6685     +113     
  Branches     6090     6191     +101     
==========================================
+ Hits         5497     5613     +116     
+ Misses        797      790       -7     
- Partials      278      282       +4

Flag	Coverage Δ
python	`95.34% <55.55%> (-1.13%)`	⬇️
rust	`83.05% <ø> (+0.42%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

doug-q

A couple comments checking my understanding, I admit I have not looked at python-pure-wheels.yml in detail, assuming it is copied from somewhere

doug-q · 2024-07-31T11:23:44Z

.release-please-manifest.json

@@ -1,3 +1,4 @@
 {
-    "tket2-py": "0.1.0a4"
+    "tket2-py": "0.1.0a4",
+    "tket2-eccs": "0.0.0"


This is correct because it matches the initial version in release-please-config.json. The robot will do the right thing here.

doug-q · 2024-07-31T11:26:39Z

pyproject.toml

 # Note: Be sure to update the dependency versions in [project.dependencies] as well
 #
 # Poetry does not currently follow PEP 621, it will be supported on poetry 2
 # https://github.com/python-poetry/poetry/issues/3332
+tket2_eccs = { path = "tket2-eccs", develop = true }


This looks strange. I think the [project] section below is what gets published to pypi, and this is what is used when you are pointing to the git repo

Yes, this is abusing the inconsistent dependency definitions (see PR linked in the comment) to work around not being able to define local path + pypi dependencies.

poetry uses tool.poetry.dependencies to define the dependency, so we tell it to use the local project (develop = true makes it so changes in tket2_eccs get reflected in the poetry env)

When the wheels get built however their metadata is computed using project.dependencies, so we reference the (currently unpublished) package there.

doug-q · 2024-07-31T11:28:29Z

.pre-commit-config.yaml

@@ -85,7 +85,8 @@ repos:
      - id: py-test
        name: pytest
        description: Run python tests
-        entry: poetry run -- sh -c "maturin develop && pytest --cov=./ --cov-report=html"
+        # Ensure that we are using the local version of `tket2-eccs` and not the one from PyPI
+        entry: poetry run -- sh -c "poetry install -C tket2-eccs & maturin develop && pytest --cov=./ --cov-report=html"


this does not look right. I would expect
poetry install -C tket2-eccs && poetry run -- sh -c "maturin develop && pytest --cov=./ --cov-report=html"

Annoyingly, pre-commit fails if you try to use && on a command

No arguments expected for "install" command, got "&&"

I realize that I wrote & instead of && though

doug-q · 2024-07-31T11:29:24Z

tket2-py/README.md

@@ -56,4 +56,4 @@ See [DEVELOPMENT.md] for more information.

 This project is licensed under Apache License, Version 2.0 ([LICENCE][] or http://www.apache.org/licenses/LICENSE-2.0).

-  [LICENCE]: ./LICENCE
+  [LICENCE]: https://github.com/CQCL/tket2/blob/main/LICENCE


I prefer the other way, if LICENSE ever changed this would be wrong.

I forgot to mention this. We need an absolute path. The LICENCE link on pypi's README is currently broken due to this.
https://pypi.org/project/tket2/

aborgna-q · 2024-07-31T14:25:19Z

I added a small drive-by to strip symbols from the compiled tket2-py lib.

The final wheel size improvement, after running maturin build --release for cp312-macosx_11_0_arm64:

	Before.	After	After (stripped)
Size	8.5MB	4.1MB	3.5MB

🤖 I have created a release *beep* *boop* --- <details><summary>tket2-py: 0.2.0</summary> ## [0.2.0](tket2-py-v0.1.0...tket2-py-v0.2.0) (2024-08-01) ### ⚠ BREAKING CHANGES * increased minimum required version of hugr to 0.9.0 * `.rwr` ECC files generated with older versions are no longer supported. Please recompile them, or compress the file with `zstd`. ### Features * Add timeout options and stats to Badger ([#496](#496)) ([32a9885](32a9885)) * Compress binary ECCs using zlib ([#498](#498)) ([d9a713c](d9a713c)) * Expose advanced Badger timeout options to tket2-py ([#506](#506)) ([fe7d40e](fe7d40e)) * Move the compiled eccs to a separate package ([#517](#517)) ([7247cc6](7247cc6)) ### Bug Fixes * Recompile ecc sets after [#441](#441) ([#484](#484)) ([1122fa4](1122fa4)) ### Miscellaneous Chores * bump hugr version to 0.10.0 ([#508](#508)) ([eca258b](eca258b)) </details> <details><summary>tket2-eccs: 0.1.0</summary> ## 0.1.0 (2024-08-01) ### Features * Move the compiled eccs to a separate package ([#517](#517)) ([7247cc6](7247cc6)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: Agustín Borgna <121866228+aborgna-q@users.noreply.github.com>

feat: Move the compiled eccs to a separate package

24b3936

aborgna-q added the run-ci-checks Special label to force running all checks in CI label Jul 30, 2024

Add the new package to the python change filters

5f1a6ff

Wheels build workflows for pure-python packages

40755f2

aborgna-q requested a review from ss2165 July 30, 2024 17:00

aborgna-q marked this pull request as ready for review July 30, 2024 17:01

aborgna-q requested review from cqc-alec and a team as code owners July 30, 2024 17:01

aborgna-q requested a review from doug-q July 31, 2024 11:24

doug-q approved these changes Jul 31, 2024

View reviewed changes

aborgna-q added 2 commits July 31, 2024 13:39

s/&/&&/

fa6fb76

drive-by: Strip debug symbols from tket2-py

81b3e7e

ss2165 approved these changes Aug 1, 2024

View reviewed changes

aborgna-q added this pull request to the merge queue Aug 1, 2024

Merged via the queue into main with commit 7247cc6 Aug 1, 2024
17 of 18 checks passed

aborgna-q deleted the ab/tket2-eccs-package branch August 1, 2024 09:07

hugrbot mentioned this pull request Aug 1, 2024

chore: release main #518

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Move the compiled eccs to a separate package #517

feat: Move the compiled eccs to a separate package #517

aborgna-q commented Jul 30, 2024 •

edited

Loading

codecov bot commented Jul 30, 2024 •

edited

Loading

doug-q left a comment

doug-q Jul 31, 2024

doug-q Jul 31, 2024

aborgna-q Jul 31, 2024 •

edited

Loading

doug-q Jul 31, 2024

aborgna-q Jul 31, 2024

doug-q Jul 31, 2024

aborgna-q Jul 31, 2024

aborgna-q commented Jul 31, 2024

feat: Move the compiled eccs to a separate package #517

feat: Move the compiled eccs to a separate package #517

Conversation

aborgna-q commented Jul 30, 2024 • edited Loading

codecov bot commented Jul 30, 2024 • edited Loading

Codecov Report

doug-q left a comment

Choose a reason for hiding this comment

doug-q Jul 31, 2024

Choose a reason for hiding this comment

doug-q Jul 31, 2024

Choose a reason for hiding this comment

aborgna-q Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

doug-q Jul 31, 2024

Choose a reason for hiding this comment

aborgna-q Jul 31, 2024

Choose a reason for hiding this comment

doug-q Jul 31, 2024

Choose a reason for hiding this comment

aborgna-q Jul 31, 2024

Choose a reason for hiding this comment

aborgna-q commented Jul 31, 2024

aborgna-q commented Jul 30, 2024 •

edited

Loading

codecov bot commented Jul 30, 2024 •

edited

Loading

aborgna-q Jul 31, 2024 •

edited

Loading