Improve developer experience by EarlMilktea · Pull Request #230 · TeamGraphix/graphix

EarlMilktea · 2024-11-01T08:32:01Z

Description of the change:

List all the CI configs. in noxfile.py
Setup pre-commit
Update ruff config.

codecov · 2024-11-01T08:35:19Z

Codecov Report

Attention: Patch coverage is 86.36364% with 6 lines in your changes missing coverage. Please review.

Project coverage is 78.66%. Comparing base (7da2095) to head (a84830d).

Files with missing lines	Patch %	Lines
graphix/gflow.py	50.00%	3 Missing ⚠️
graphix/pattern.py	75.00%	2 Missing ⚠️
graphix/device_interface.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #230 +/- ##
==========================================
- Coverage 78.99% 78.66% -0.34% 
==========================================
Files 40 40 Lines 6113 5976 -137 ==========================================
- Hits 4829 4701 -128 + Misses 1284 1275 -9

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

thierry-martinez

Thanks. Some minor remarks.

Co-authored-by: thierry-martinez <thierry.martinez@inria.fr>

shinich1

thanks! could you add instruction to use pre-commit in CONTRIBUTING.md? brief description with pointing to a good online page is ok. anything else we need to elaborate in the contribution guide?

EarlMilktea · 2024-11-04T15:22:20Z

CONTRIBUTING.md is already updated to use pre-commit instead of ruff (may need to add more though).

EarlMilktea · 2024-11-10T11:16:26Z

@thierry-martinez

thierry-martinez

There is a regression in to_qasm3, and some minor remarks.

thierry-martinez · 2024-11-25T16:58:54Z

+ hooks:
+ - id: ruff
+ name: ruff-isort
+ args: [--fix, --select, I]


I believe the developer experience will improve without --fix:
When executed as a GitHub Action, messages for fixed errors are hidden, rendering the output unhelpful.
When executed locally, making changes to the unstaged area during a commit operation is counterintuitive. Developers are better off explicitly running ruff --fix to apply auto-fix changes when errors are reported.

When executed as a GitHub Action, messages for fixed errors are hidden, rendering the output unhelpful.
When executed locally, making changes to the unstaged area during a commit operation is counterintuitive. Developers are better off explicitly running ruff --fix to apply auto-fix changes when errors are reported.
Agree, but if so I feel pre-commit may not be the best choice as its strength comes from env. isolation feature (it does not contaminate venv with CI deps.) while your comments suggest that we'd better invoke ruff manually by hand.

We can have the best of both worlds: pre-commit is useful for running local checks automatically and ensuring consistency with CI, while manually running ruff --fix is helpful for applying trivial fixes quickly. Under the rules proposed in this PR, developers would still need to invoke ruff --fix to automatically address errors outside the I group.
I believe it’s best to start with the single hook below, which offers clearer error messages and fewer surprises, and then leave room for further improvements if we later agree on additional hooks.
- id: ruff

thierry-martinez · 2024-11-25T17:00:34Z

+ - id: ruff
+ name: ruff-isort
+ args: [--fix, --select, I]
+ - id: ruff


Why separate I from the rest? If the sole reason is --fix, I believe we can safely merge the two rules by removing --fix.

This mimics the default behavior of isort.

This thread is related to the above discussion: #230 (comment)

thierry-martinez · 2024-11-25T17:03:18Z

+pre-commit run -a
 ```

 and you are ready to commit the changes.


We could add the note: 'If ruff check reports that some errors can be automatically fixed, you can rerun it with the --fix option: ruff check --fix.'

This thread is related to the above discussion: #230 (comment)

thierry-martinez · 2024-11-26T13:11:03Z

+files-typed: &files-typed "^(\
+ graphix/channels|\
+ graphix/clifford|\
+ graphix/command|\
+ graphix/fundamentals|\
+ graphix/instruction|\
+ graphix/linalg_validations|\
+ graphix/measurements|\
+ graphix/ops|\
+ graphix/pauli|\
+ graphix/pyzx|\
+ graphix/rng|\
+ graphix/states|\
+ graphix/utils|\
+ graphix/_db|\
+ noxfile|\
+ setup.py|\
+ tests/conftest|\
+ tests/test_clifford|\
+ tests/test_command|\
+ tests/test_db|\
+ tests/test_fundamentals|\
+ tests/test_kraus|\
+ tests/test_pauli|\
+ tests/test_pyzx|\
+ tests/test_rng\
+ .*/__init__|\
+ ).py$"
+
+deps-typecheck:
+ &deps-typecheck [
+ "nox",
+ "numpy",
+ "psutil",
+ "pydantic",
+ "pytest",
+ "pytest-mock",
+ "pyzx",
+ "types-networkx",
+ "types-psutil",
+ ]


Defining files-typed and deps-typecheck at root triggers the following warnings:
[WARNING] Unexpected key(s) present at root: deps-typecheck, files-typed
I propose to inline them in the first occurrence of their use: 17bbdd9

Co-authored-by: thierry-martinez <thierry.martinez@inria.fr>

In the discussion on TeamGraphix#230, there was a comment about improving the device interface implementation (see TeamGraphix#230 (comment) ). In particular, I removed the `except Exception` path, being unable to reproduce an example where this path is reached. If we want to handle some errors (such as `TranspilerError`?), we should be more specific.

EarlMilktea · 2025-01-25T14:36:44Z

Let me close this PR as I no longer feel this PR will contribute to better DX.

EarlMilktea added 6 commits November 1, 2024 15:56

🧑‍💻 Move tests to noxfile
95de334

👷 Update testing CI
3cc80a7

🧑‍💻 Partially setup pre-commit
5cf0b11

🚨 Fix by pre-commit
a3adf42

👷 Update CI
d8a509f

🧑‍💻 Update pre-commit automatically
6ec0a15

EarlMilktea self-assigned this Nov 1, 2024

EarlMilktea added 5 commits November 1, 2024 17:35

👷 Install deps. for type checking
ab15f58

🧑‍💻 Migrate typecheck to pre-commit
bce264b

🧑‍💻 Autofix
7cd2bfe

🩹 Minor fixes
a2b5b83

➕ Add dep.
924a69c

EarlMilktea force-pushed the improve-dx branch from 582c020 to 924a69cCompare November 1, 2024 09:39

EarlMilktea added 9 commits November 1, 2024 18:41

🔧 Enforce F401
3c491c4

⏪ Remove isinstance check
31963d0

♻️ Enforce __future__ annotation
7f08661

🔧 Migrate to blacklist-based config
d359e4e

🚨 Autofix
ff72b85

🚨 Fix manually
cd6ebaa

🔧 Enable preview
ce4da40

💩 Ignore errors tentatively
b41dbb6

🚨 Autofix
9498955

EarlMilktea force-pushed the improve-dx branch from 9328828 to f2b1858Compare November 1, 2024 11:40

🚨 Manually fix
237a5b9

EarlMilktea force-pushed the improve-dx branch from f2b1858 to 237a5b9Compare November 1, 2024 11:45

EarlMilktea requested review from shinich1 and thierry-martinez November 1, 2024 11:55

EarlMilktea marked this pull request as ready for review November 1, 2024 11:55

thierry-martinez requested changes Nov 1, 2024
View reviewed changes

Comment threadgraphix/gflow.py Outdated
Comment threadgraphix/gflow.py Outdated
Comment threadgraphix/sim/density_matrix.py

♻️ Use dict literal
f02be10
Co-authored-by: thierry-martinez <thierry.martinez@inria.fr>

EarlMilktea force-pushed the improve-dx branch from e760dca to f02be10Compare November 1, 2024 14:49

EarlMilktea requested a review from thierry-martinez November 1, 2024 14:50

EarlMilktea added 3 commits November 2, 2024 01:47

🧑‍💻 Only fix I
273b220

🔀 Merge branch 'master' into improve-dx
12df44e

🚨 Specify error type
86d4e1e

EarlMilktea force-pushed the improve-dx branch from 470ac7b to 86d4e1eCompare November 4, 2024 09:25

shinich1 reviewed Nov 4, 2024
View reviewed changes

EarlMilktea requested a review from shinich1 November 4, 2024 15:30

shinich1 approved these changes Nov 4, 2024
View reviewed changes

thierry-martinez requested changes Nov 26, 2024
View reviewed changes

🐛 Fix broken file path
a84830d
Co-authored-by: thierry-martinez <thierry.martinez@inria.fr>

This was referenced Jan 6, 2025
Cleaning up device interface #241
Closed
[Bug]: ruff issues #242
Closed

EarlMilktea closed this Jan 25, 2025

EarlMilktea deleted the improve-dx branch January 25, 2025 14:36

Uh oh!

Conversation

EarlMilktea commented Nov 1, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecovBot commented Nov 1, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thierry-martinez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shinich1 left a comment

Choose a reason for hiding this comment

Uh oh!

EarlMilktea commented Nov 4, 2024

Uh oh!

EarlMilktea commented Nov 10, 2024

Uh oh!

thierry-martinez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EarlMilktea commented Jan 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

EarlMilktea commented Nov 1, 2024•
edited
Loading

codecovBot commented Nov 1, 2024•
edited
Loading