Nailing down automatic circuit cutter's API #2168

zeyueN · 2022-02-04T03:43:02Z

Context:
When passing a tape to the automatic circuit cutter (KaHyPar), several hyperparameters (e.g. k, imbalance, etc.) need to be supplied. Depending on use cases, sensible default values should be derived based on known information (which usually comes at different points in time) such as the available devices and the circuits. However, power users should also be able to override and supply their own parameters. We thus must provide a unified interface that's general enough to allow these types of interactions.

Drawing similarities between distributing circuit fragments to devices and the classical problem of distributing computation tasks dags to multiple workers, the constraints coming from devices should be separated from the requirements coming from the computations. This separation is realized inside the added class CutStrategy, whose attributes contain constraints from devices and whose method get_cut_kwargs() takes circuit requirements at runtime and returns complete sets of cutting hyperparameters. In terms of UI, both the device constrains and the calling of get_cut_kwargs() could be done either manually by users or within decorators. dataclass was elected to make the implementation of the init function cleaner.

Description of the Change:
Adds:

a class CutStrategy that can be used as an interface to coordinate devices info, user constraints, and circuit requirements into partition hyperparameters ready for partitioning methods.

Possible Drawbacks:
Gate cut compatibility not fully explored

- helper function for deriving best default cut parameters - 2 container dataclesses (CutConfig and CutSpec) for automatic cutter's input and output.

github-actions · 2022-02-04T03:43:17Z

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

A one-to-two sentence description of the change. You may include a small working example for new features.
A link back to this PR.
Your name (or GitHub username) in the contributors section.

codecov · 2022-02-04T04:17:36Z

Codecov Report

Merging #2168 (0f738cc) into master (60bdfd1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master    #2168    +/-   ##
========================================
  Coverage   99.26%   99.26%            
========================================
  Files         231      231            
  Lines       18228    18330   +102     
========================================
+ Hits        18094    18196   +102     
  Misses        134      134

Impacted Files	Coverage Δ
pennylane/transforms/__init__.py	`100.00% <ø> (ø)`
pennylane/transforms/qcut.py	`100.00% <100.00%> (ø)`

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 60bdfd1...0f738cc. Read the comment docs.

trbromley

Thanks @zeyueN!

pennylane/transforms/qcut.py

trbromley · 2022-02-11T13:48:51Z

[sc-14728]

zeyueN · 2022-02-15T15:48:36Z

Edited the description. Promoting to real PR.

trbromley

Thanks @zeyueN! I left a few suggestions/comments, but overall this looks great and happy to approve once these are resolved.

pennylane/transforms/qcut.py

doc/releases/changelog-dev.md

pennylane/transforms/qcut.py

trbromley · 2022-02-17T15:32:08Z

pennylane/transforms/qcut.py

+            ``devices`` is provided where it defaults to the maximum number of wires among
+            ``devices``.
+        min_free_wires (int): Number of wires for the smallest available device.
+            Optional, defaults to ``max_device_wires``.


It might be good to add a sentence explaining the implication of setting this number not to be default.

pennylane/transforms/qcut.py

trbromley · 2022-02-17T16:42:49Z

pennylane/transforms/qcut.py

+        **Example**
+
+        .. code-block:: python
+
+            import pennylane as qml
+            from pennylane.transforms import qcut
+
+            dev = qml.device('default.qubit', wires=4)
+
+            with qml.tape.QuantumTape() as tape:
+                qml.RX(0.4, wires=0)
+                qml.RY(0.9, wires=0)
+                qml.CNOT(wires=[0, 1])
+                qml.expval(qml.PauliZ(1))
+
+            tape_dag = qcut.tape_to_graph(tape)
+
+        Deriving kwargs for a given circuit and feed it to a custom partitioner along with an extra
+        custom parameter:
+        >>> cut_strategy = qcut.CutStrategy(devices=dev)
+        >>> cut_kwargs = cut_strategy.get_cut_kwargs(tape_dag)
+        >>> cut_kwargs.update({'extra_param': 0})
+        >>> my_partitioner(tape_dag, **cut_kwargs)


Would be good to move the example below args and returns

trbromley · 2022-02-17T16:43:51Z

pennylane/transforms/qcut.py

+        Deriving kwargs for a given circuit and feed it to a custom partitioner along with an extra
+        custom parameter:
+        >>> cut_strategy = qcut.CutStrategy(devices=dev)
+        >>> cut_kwargs = cut_strategy.get_cut_kwargs(tape_dag)
+        >>> cut_kwargs.update({'extra_param': 0})
+        >>> my_partitioner(tape_dag, **cut_kwargs)


Perhaps this section is the only bit we need.

trbromley · 2022-02-17T16:46:10Z

pennylane/transforms/qcut.py

+            List[Dict[str, Any]]: A list of minimal set of kwargs being passed to a graph
+                partitioner method.


I think you might have answered in previous comments, but why a list? Also it seems at odds with the example:

>>> cut_kwargs = cut_strategy.get_cut_kwargs(tape_dag) >>> cut_kwargs.update({'extra_param': 0}) >>> my_partitioner(tape_dag, **cut_kwargs)

The returned list contains all potential cut kwargs to explore, i.e. different ks and imbalances. You are right the example is wrong, have fixed it.

trbromley · 2022-02-17T16:50:22Z

pennylane/transforms/qcut.py

+            assert isinstance(max_wires_by_fragment, (list, tuple))
+            assert all(isinstance(i, int) and i > 0 for i in max_wires_by_fragment)


Are asserts the best choice for user validation? It's more length by if-raise seems more natural.

pennylane/transforms/qcut.py

anthayes92 · 2022-02-18T16:39:44Z

pennylane/transforms/qcut.py

+    ):
+        """Deriving cutting constraints from given devices and parameters."""
+
+        self.max_free_wires = self.max_free_wires or self.min_free_wires


I don't really understand what this is doing, why is there the or self.min_free_wires here?

Couldn't we just have these values set as in lines 617 and 618?

This is me wanting to be slightly more forgiving before checking if one of devices and max_free_wires is provided, where if neither are provided but min_free_wires are, then I interpret max_free_wires to be the same as min_free_wires. Unlikely situation, but if it happens this interpretation should be safe.

Thanks this makes sense, though now I'm not sure I understand the docstring where for max_free_wires we have "Optional only when ``devices`` is provided" and vice versa for devices. Maybe I'm misunderstanding the use of Optional but this seems to suggest that at least one of the two must be passed?

You are correct that the docstring doesn't accurately describe this behaviour. This was intentional since this is sort of a "failsafe" behaviour which doesn't need to be advertised to users since it adds nothing functionally new and could potentially confuse people even more. In the end, I do want to force users to provide either devices or max_free_wires.

pennylane/transforms/qcut.py

anthayes92 · 2022-02-18T17:15:49Z

pennylane/transforms/qcut.py

+        if max_wires_by_fragment is not None and max_gates_by_fragment is not None:
+            assert len(max_wires_by_fragment) == len(max_gates_by_fragment)


why do these need to be of equal length?

Because the length implicitly signifies the number of fragments, i.e. k.

anthayes92 · 2022-02-18T19:23:04Z

pennylane/transforms/qcut.py

+        assert free_wires >= avg_fragment_wires
+        assert free_gates >= avg_fragment_gates


I think free_wires can be a user defined variable (max_wires_by_fragment) so this might be well suited to raising an exception. Same for gates.

Done. Also added a test since these raises won't be able to get triggered by user facing functions due to all the checks before the function is invoked.

Co-authored-by: anthayes92 <34694788+anthayes92@users.noreply.github.com>

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

Co-authored-by: anthayes92 <34694788+anthayes92@users.noreply.github.com>

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

…om/PennyLaneAI/pennylane into sc-14145-decide-upon-optimizer-api

zeyueN · 2022-02-22T17:14:35Z

@trbromley I think I've addressed all the comments and ready for review.

~~The documentation check workflow is failing for some reason. Any idea how to fix?~~

trbromley

Thanks @zeyueN, just a few more comments.

doc/releases/changelog-dev.md

pennylane/transforms/qcut.py

trbromley · 2022-02-24T19:36:13Z

pennylane/transforms/qcut.py

+                (not isinstance(d, qml.Device) for d in devices)
+            ):
+                raise ValueError(
+                    f"Argument `devices` contain at least one `Device` instance, got {type(devices)}."


Is this the right error message? We raise an error if:

Devices is not a list of tuple, or

Any element in device is not a qml.Device

Maybe the error message should just read:

Argument `devices` must be a list or tuple containing elements of type qml.Device

(Also, should we just check for Sequence rather than list/tuple?)

Ya definitely screwed up that sentence, switched to your suggestion.

pennylane/transforms/qcut.py

trbromley · 2022-02-24T19:53:38Z

pennylane/transforms/qcut.py

+    devices: InitVar[Sequence[qml.Device]] = None
+    max_free_wires: int = None
+    min_free_wires: int = None
+    num_fragments_probed: Union[int, Sequence[int]] = None
+    max_free_gates: int = None
+    min_free_gates: int = None
+    imbalance_tolerance: float = None


Since the docs shows these attributes without docstrings:

Would it be possible to add quick one line explanations? E.g.,

imbalance_tolerance: float = None #: The global maximum allowed imbalance for all partition trials

(I'm not completely sure on the UI to get this to show up in the docs so might need some trial and error)

Done, the comment should be above the line.

Awesome, thanks for figuring out the right orientation for the comment!

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

trbromley

Awesome, looks great - thanks @zeyueN! I left some final comments, but otherwise good to go! Approved 🟢

pennylane/transforms/qcut.py

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

Draft implementation of

72fc2ab

- helper function for deriving best default cut parameters - 2 container dataclesses (CutConfig and CutSpec) for automatic cutter's input and output.

zeyueN self-assigned this Feb 4, 2022

zeyueN added 2 commits February 4, 2022 03:52

linting

26251f5

moves assets up, naive collect wire

7e25adc

trbromley reviewed Feb 4, 2022

View reviewed changes

zeyueN added 15 commits February 11, 2022 21:49

refactor infer defaults as class method to CutConfig

5d39fea

linting

1ab2771

minor fix

c19171d

Merge branch 'master' into sc-14145-decide-upon-optimizer-api

8df7b1b

refactor, new CutConstraints class

1fcac43

renames to CutStrategy, docstrings

037007d

linting

d52ede3

adds tests

3d9847d

get_args test

b751061

more tests

320151a

code factor

0336de6

adds coverage

b28b73e

rm black chgs

0df8210

Merge branch 'master' into sc-14145-decide-upon-optimizer-api

59325ea

clean comments

03dd3b4

zeyueN marked this pull request as ready for review February 15, 2022 15:47

chglog

c00bcde

zeyueN requested review from anthayes92 and trbromley February 15, 2022 15:55

Merge branch 'master' into sc-14145-decide-upon-optimizer-api

39c68c9

trbromley reviewed Feb 17, 2022

View reviewed changes

Merge branch 'master' into sc-14145-decide-upon-optimizer-api

745fc14

anthayes92 reviewed Feb 18, 2022

View reviewed changes

zeyueN and others added 9 commits February 22, 2022 01:54

Merge branch 'master' into sc-14145-decide-upon-optimizer-api

bdb43d3

Update pennylane/transforms/qcut.py

39ef017

Co-authored-by: anthayes92 <34694788+anthayes92@users.noreply.github.com>

Update pennylane/transforms/qcut.py

00dba22

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

Update pennylane/transforms/qcut.py

0ffeae1

Co-authored-by: anthayes92 <34694788+anthayes92@users.noreply.github.com>

Update pennylane/transforms/qcut.py

bebc6c0

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

apply pr comments

1068916

Merge branch 'sc-14145-decide-upon-optimizer-api' of https://github.c…

ac937a7

…om/PennyLaneAI/pennylane into sc-14145-decide-upon-optimizer-api

asserts to raises

eb823e2

Merge branch 'master' into sc-14145-decide-upon-optimizer-api

0eaf85d

trbromley self-requested a review February 22, 2022 17:55

zeyueN added 5 commits February 22, 2022 18:59

trim example

8049a8d

improve cov

02c5293

Merge branch 'master' into sc-14145-decide-upon-optimizer-api

cea2a65

rm doc

5648843

fixes docstring

ebcc24e

trbromley reviewed Feb 24, 2022

View reviewed changes

zeyueN and others added 4 commits February 24, 2022 15:31

Apply suggestions from code review

b5eab8d

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

device value error raise

e82a5f4

adds docstring for attributes

033e5eb

Merge branch 'master' into sc-14145-decide-upon-optimizer-api

35c8b8c

zeyueN requested a review from trbromley February 24, 2022 22:57

trbromley approved these changes Feb 24, 2022

View reviewed changes

zeyueN and others added 3 commits February 24, 2022 18:27

Apply suggestions from code review

8fc3419

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

rm fstring

8238ea2

format

0f738cc

zeyueN merged commit 1c0a57c into master Feb 25, 2022

zeyueN deleted the sc-14145-decide-upon-optimizer-api branch February 25, 2022 00:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nailing down automatic circuit cutter's API #2168

Nailing down automatic circuit cutter's API #2168

zeyueN commented Feb 4, 2022 •

edited

Loading

github-actions bot commented Feb 4, 2022

codecov bot commented Feb 4, 2022 •

edited

Loading

trbromley left a comment

trbromley commented Feb 11, 2022

zeyueN commented Feb 15, 2022

trbromley left a comment

trbromley Feb 17, 2022

trbromley Feb 17, 2022

trbromley Feb 17, 2022

trbromley Feb 17, 2022

zeyueN Feb 22, 2022

trbromley Feb 17, 2022

zeyueN Feb 22, 2022

anthayes92 Feb 18, 2022 •

edited

Loading

zeyueN Feb 22, 2022

anthayes92 Feb 23, 2022 •

edited

Loading

zeyueN Feb 23, 2022

anthayes92 Feb 18, 2022

zeyueN Feb 22, 2022

anthayes92 Feb 18, 2022

zeyueN Feb 22, 2022

zeyueN commented Feb 22, 2022 •

edited

Loading

trbromley left a comment

trbromley Feb 24, 2022

zeyueN Feb 24, 2022

trbromley Feb 24, 2022

zeyueN Feb 24, 2022

trbromley Feb 24, 2022

trbromley left a comment

		List[Dict[str, Any]]: A list of minimal set of kwargs being passed to a graph
		partitioner method.

		assert isinstance(max_wires_by_fragment, (list, tuple))
		assert all(isinstance(i, int) and i > 0 for i in max_wires_by_fragment)

		if max_wires_by_fragment is not None and max_gates_by_fragment is not None:
		assert len(max_wires_by_fragment) == len(max_gates_by_fragment)

		assert free_wires >= avg_fragment_wires
		assert free_gates >= avg_fragment_gates

Nailing down automatic circuit cutter's API #2168

Nailing down automatic circuit cutter's API #2168

Conversation

zeyueN commented Feb 4, 2022 • edited Loading

github-actions bot commented Feb 4, 2022

codecov bot commented Feb 4, 2022 • edited Loading

Codecov Report

trbromley left a comment

Choose a reason for hiding this comment

trbromley commented Feb 11, 2022

zeyueN commented Feb 15, 2022

trbromley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthayes92 Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthayes92 Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeyueN commented Feb 22, 2022 • edited Loading

trbromley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trbromley left a comment

Choose a reason for hiding this comment

zeyueN commented Feb 4, 2022 •

edited

Loading

codecov bot commented Feb 4, 2022 •

edited

Loading

anthayes92 Feb 18, 2022 •

edited

Loading

anthayes92 Feb 23, 2022 •

edited

Loading

zeyueN commented Feb 22, 2022 •

edited

Loading