Refactoring API structure by d1ssk · Pull Request #19 · TeamGraphix/graphix-ibmq

d1ssk · 2025-03-31T13:19:20Z

Before submitting, please check the following:

Make sure you have tests for the new code and that test passes (run tox)
format added code by black -l 120 <filename>
If applicable, add a line to the [unreleased] part of CHANGELOG.md, following keep-a-changelog.

Then, please fill in below:

Context (if applicable):
This PR aligns with the refactored backend interface structure introduced in graphix PR /#261

Description of the change:

Implemented IBMQBackend.
Implemented IBMQJob.
Refactored and separated concerns:
- compiler.py: converts Pattern to QuantumCircuit
- executor.py: handles both real hardware execution and Aer simulation
- job.py: manages submitted job.
- result_utils.py: extracts and formats output from raw IBMQ results
- compile_options.py: defines IBMQCompileOptions dataclass for type-safe parameter handling
backend.py: orchestrates the entire workflow from compile → submit → retrieve

Related issue:

also see that checks (github actions) pass.
If lint check keeps failing, try installing black==22.8.0 as behavior seems to vary across versions.

graphix_ibmq/backend.py

graphix_ibmq/job.py

graphix_ibmq/result_utils.py

EarlMilktea · 2025-05-26T18:00:07Z

tests/test_backend.py

@@ -0,0 +1,56 @@
+import pytest


I strongly recommend you add type annotations, at least to newly-added files.

Please do not omit return type annotations.

.github/workflows/ci.yml

shinich1 · 2025-07-02T16:38:27Z

setup.py

    exec(fp.read(), version)

-requirements = [requirement.strip() for requirement in open("requirements.txt").readlines()]
+# requirements = [requirement.strip() for requirement in open("requirements.txt").readlines()]


Why was this line removed in favor of the explicit definition of requirements in the following line? I think the previous version of the code, where requirements was obtained from the contents of requirements.txt, was preferable, since it allows us not to duplicate the list of the dependencies in both setup.py and requirements.txt.

Note that there is already an inconsistency resulting from this duplication: graphix is listed here but is missing in requirements.txt.

I think we should plan, probably not in this PR, but in a subsequent PR, to move the declarative contents of setup.py to pyproject.toml and remove setup.py.

thierry-martinez

Some fixes should be made for this PR to work, now that TeamGraphix/graphix#261 has been merged.

thierry-martinez · 2025-07-04T10:30:37Z

setup.py

    exec(fp.read(), version)

-requirements = [requirement.strip() for requirement in open("requirements.txt").readlines()]
+# requirements = [requirement.strip() for requirement in open("requirements.txt").readlines()]


Why was this line removed in favor of the explicit definition of requirements in the following line? I think the previous version of the code, where requirements was obtained from the contents of requirements.txt, was preferable, since it allows us not to duplicate the list of the dependencies in both setup.py and requirements.txt.

Note that there is already an inconsistency resulting from this duplication: graphix is listed here but is missing in requirements.txt.

I think we should plan, probably not in this PR, but in a subsequent PR, to move the declarative contents of setup.py to pyproject.toml and remove setup.py.

thierry-martinez · 2025-07-04T10:31:11Z

requirements.txt

+numpy
 qiskit>=1.0
 qiskit_ibm_runtime
 qiskit-aer


graphix is probably missing here.

Thank you for the comments. I had temporarily modified the code to use a specific branch of graphix and forgot to revert it; I’ve now fixed it in 9cade0e.

I agree with the plan to make other PR to move the declarative contents of setup.py into pyproject.toml and remove setup.py.

thierry-martinez · 2025-07-04T10:43:25Z

tox.ini

 description = Run the unit tests
 deps =
    -r {toxinidir}/requirements.txt
+    git+https://github.com/TeamGraphix/graphix@device_interface_abc#egg=graphix


Note that the branch device_interface_abc no longer exists since TeamGraphix/graphix#261 was merged.

Whether or not we rely on a particular branch of graphix, I think the dependency on graphix should not be made explicit here, but in requirements.txt, since users could want to install dependencies directly with pip install -r, and not necessarily with tox.

Fixed in 9cade0e

thierry-martinez · 2025-07-04T10:52:02Z

tests/test_compiler.py

+from qiskit import transpile
+from qiskit_aer import AerSimulator
+from graphix_ibmq.compiler import IBMQPatternCompiler
+import random_circuit as rc


random_circuit no longer exists: you probably want to use graphix.random_objects.

thierry-martinez · 2025-07-04T10:58:11Z

.github/workflows/ci.yml

      matrix:
        os: ['ubuntu-latest', 'windows-2022', 'macos-latest']
-        python: ['3.8', '3.9', '3.10', '3.11']
+        python: ['3.9', '3.10', '3.11', '3.12']


These versions of Python are not really used: regardless of the version of Python we test here, tox installs and tests Python 3.9, 3.10, and 3.11 in its own environments. That means we run the exact same set of tests four times, and each time we test the three versions of Python listed in tox.ini. We probably want to test only one version of Python at this level (and probably even without explicitly specifying a particular version), and let tox run the tests in its multiple environments.

Thanks for the comment, but actually I don't think the current setup runs every tox env four times. Each github actions job only invokes its corresponding tox environment (e.g. the 3.10 runner only runs py310, the 3.11 runner only runs py311, etc.), although running tox in local environment indeed execute tests in all the versions.

Oh yes, sorry: I was mistaken, you're right. That is the purpose of the [gh-actions] section in tox.ini. Then everything is fine!

thierry-martinez · 2025-07-04T11:03:35Z

graphix_ibmq/backend.py

+    """IBMQ backend implementation for compiling and executing quantum patterns."""
+
+    _options: IBMQCompileOptions
+    _compiled_circuit: QuantumCircuit | None


_compiled_circuit and _compiler should not be part of the internal state of the backend, because we may want to compile several circuits with the same backend instance without losing the previous circuits each time we compile a new one. It would be better if we had a class to represent IBMQCompiledCircuit, and the method compile would return an instance of this class rather than storing the circuit in the internal state of the backend.

Thank you for the comment. Refactored accordingly in d2b6035

thierry-martinez · 2025-07-04T11:04:40Z

graphix_ibmq/job.py

+    from graphix_ibmq.compiler import IBMQPatternCompiler
+
+
+class IBMQJob:


This class could be implemented as a dataclass.

Thank you for the comment. Refactored accordingly in d2b6035

thierry-martinez · 2025-07-04T11:06:20Z

graphix_ibmq/job.py

+    def __init__(
+        self,
+        job: LocalRuntimeJob | RuntimeJobV2,
+        compiler: IBMQPatternCompiler,


We don't need to know the compiler at this level; we only need to know the pattern and the register dictionary used by retrieve_result.

Thank you for the comment. Refactored in d2b6035 so that IBMQJob class has IBMQCompliedCircuit instance instead of compiler instance to access pattern or register dictionary information.

thierry-martinez

LGTM! Thanks.

thierry-martinez · 2025-07-16T15:01:26Z

.github/workflows/ci.yml

      matrix:
        os: ['ubuntu-latest', 'windows-2022', 'macos-latest']
-        python: ['3.8', '3.9', '3.10', '3.11']
+        python: ['3.9', '3.10', '3.11', '3.12']


Oh yes, sorry: I was mistaken, you're right. That is the purpose of the [gh-actions] section in tox.ini. Then everything is fine!

EarlMilktea · 2025-07-16T17:14:39Z

@d1ssk

General comments:

I'm seeing trivial type errors that can be resolved readily with mypy / pyright. I suggest resolving them early in refactoring steps.
Using None as default values in class is, I believe, in general bad because you need to write is None or similar type narrowing branches each time you want to do something meaningful. Why not initialize them in __init__ ?
Input type annotations must be thoroughly expressed in collections.abc instances rather than dict / list etc.
Please do not define get_xxx or set_xxx : use @property instead. If your set_xxx has additional arguments it might also be a bad design as setting something is inherently free from additional information.
Please do not omit argument/return type annotations even when they are just plain -> None .

d1ssk · 2025-07-28T16:15:51Z

@EarlMilktea Thank you for the review. I implemented your suggestions.

Resolved type errors: All type errors previously reported by mypy have been resolved.
Refactored IBMQBackend to eliminate None initialization and set_xxx methods.
- Replaced the set_simulator() and set_hardware() methods with factory classmethods (from_simulator() and from_hardware()).
Use of collections.abc instances for Inputs
Completed Type Annotations

Also, I replaced all print() with the standard logging module.

graphix_ibmq/backend.py

requirements.txt

graphix_ibmq/backend.py

graphix_ibmq/compiler.py

graphix_ibmq/job.py

d1ssk added 13 commits March 31, 2025 22:14

refactor API structure

05ddfc9

add docstring

cfc7d3d

add tests

cc96047

edit setup.py

94f5506

update tox.ini

edd098b

update requirement.txt

d704a07

add py3.12, remove py3.8 in test and support

7be6452

edit ci.yml

4d695d6

black

edf5f22

update tox.ini

66154e7

update tox.ini

5621900

remove py3.12 from support and test

9ebab22

remove py3.12 from support and test

ddfaff6

shinich1 requested a review from thierry-martinez May 12, 2025 11:25

d1ssk added 3 commits May 26, 2025 18:30

delete abstract class in graphix

ad49e67

update test

c4748e7

update test

9d9adf1

d1ssk requested a review from shinich1 May 26, 2025 12:11

EarlMilktea requested changes May 26, 2025

View reviewed changes

thierry-martinez mentioned this pull request May 28, 2025

Reverse-dependency checks TeamGraphix/graphix#285

Closed

d1ssk added 2 commits June 20, 2025 21:31

refactor according to the review

b153822

unpin dependencies

409ae3c

shinich1 reviewed Jul 2, 2025

View reviewed changes

shinich1 approved these changes Jul 2, 2025

View reviewed changes

thierry-martinez requested changes Jul 4, 2025

View reviewed changes

d1ssk added 5 commits July 8, 2025 18:29

edit setup and ci

9cade0e

remove python version specification in ci.yml

5396ce4

remove python setup in ci.yml

fcbfe34

recover ci.yml

f4353bd

refactor

d2b6035

black

b655ce0

thierry-martinez approved these changes Jul 16, 2025

View reviewed changes

Redesign IBMQBackend for improved type safety

aa713a2

EarlMilktea requested changes Aug 13, 2025

View reviewed changes

d1ssk added 5 commits August 15, 2025 21:48

reflect the review

8b533a1

Ensure type safety

71f4359

black

c1074ae

add test fot job.py

c6f94d1

black

ae3882f

EarlMilktea approved these changes Sep 17, 2025

View reviewed changes

d1ssk merged commit b995d07 into master Sep 22, 2025
12 checks passed

		from graphix_ibmq.compiler import IBMQPatternCompiler


		class IBMQJob:

Conversation

d1ssk commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

d1ssk Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

d1ssk Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

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!

EarlMilktea commented Jul 16, 2025

Uh oh!

d1ssk commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

d1ssk commented Mar 31, 2025 •

edited

Loading

d1ssk Jul 14, 2025 •

edited

Loading

d1ssk Jul 14, 2025 •

edited

Loading

d1ssk commented Jul 28, 2025 •

edited

Loading