Consolidate `Target` mappings by raynelfss · Pull Request #15349 · Qiskit/qiskit

raynelfss · 2025-11-17T20:00:22Z

Summary

The following commits remove the redundant mappings gate_name_map and angle_bounds in favor of one mapping with a struct that contains all of that data. This results in a much lighter Target going from 504 (0x1F8) bytes to 288 bytes.

The new struct called TargetProperties is currently internal-only. And contains three fields.

properties containing the mapping of the qargs and instruction properties (originally the gate_map).
instruction containing the TargetOperation (originally the gate_name_map).
angle_bounds: Optionally containing the angle bounds for the Target (originally angle_bounds.)

This is the first effort to more efficiently use the Target in Rust with upcoming efforts focusing on its behavior on Python.

Details and comments

qiskit-bot · 2025-11-17T20:00:26Z

One or more of the following people are relevant to this code:

@Qiskit/terra-core

coveralls · 2025-11-17T20:45:41Z

Pull Request Test Coverage Report for Build 21007639926

Details

140 of 168 (83.33%) changed or added relevant lines in 2 files are covered.
10 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.01%) to 88.328%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/target.py	0	1	0.0%
crates/transpiler/src/target/mod.rs	140	167	83.83%

Files with Coverage Reduction	New Missed Lines	%
crates/circuit/src/parameter/parameter_expression.rs	1	82.3%
crates/transpiler/src/passes/unitary_synthesis.rs	1	93.32%
crates/qasm2/src/lex.rs	2	93.32%
crates/transpiler/src/target/mod.rs	6	84.77%

Totals
Change from base Build 21005190367:	0.01%
Covered Lines:	97023
Relevant Lines:	109844

💛 - Coveralls

The following commits remove the redundant mappings `gate_name_map` and `angle_bounds` in favor of one mapping with a struct that contains all of that data. This results in a much lighter `Target` going from 504 (0x1F8) bytes to 288 bytes. The new struct called `TargetProperties` is currently internal-only. And contains three fields. - `properties` containing the mapping of the qargs and instruction properties (originally the `gate_map`). - `instruction` containing the `TargetOperation` (originally the `gate_name_map`). - `angle_bounds`: Optionally containing the angle bounds for the ``Target`` (originally `angle_bounds`.) This is the first effort to more efficiently use the ``Target`` in Rust with upcoming efforts focusing on its behavior on Python.

mtreinish

Overall I really like the structural change here, it reduces the weight of the target more than you pointed out in the PR summary message. That only looks mechanically at the data in the struct itself, not the allocations being pointed too. I expect for a typical target this will shrink the memory footprint more substantially. You've eliminated 2 string copies per instruction in the target. Did you run any benchmarks with this to see if it improved things at all? Also did we have an issue about reducing the string copies in the target at one point? If we did we should mark this as closing it.

I just had a few inline comments and questions.

mtreinish · 2026-03-19T03:44:35Z

+    global_operations: HashMap<u32, HashSet<String>>,
+    qarg_gate_map: HashMap<Qargs, HashSet<String>>,


I assume these were an indexmap previously for deterministic iteration order which would have been important for Python backwards compatibility (these were originally python dicts I think). Are these not exposed directly in the public api for the target, or were there no methods to iterate them and that's why it's safe to change?

Yes, in most cases I noticed the iteration was either internal only for classification, or the iteration was exposed to Python as a set, which in Python is unordered. For example, the Target::qargs method which is the only one exposed to python that iterates through one of these maps qarg_gate_map, wraps everything inside of a set, while the rust version returns an iterator.

mtreinish · 2026-03-19T03:45:19Z

    pub qubit_properties: Option<Vec<QubitProperties>>,
    #[pyo3(get, set)]
    pub concurrent_measurements: Option<Vec<Vec<PhysicalQubit>>>,
-    gate_map: GateMap,


I'm very happy to see this type alias go away, I always found the type aliases got in the way, and seeing that this is an Indexmap with the types is much easier to reason about.

mtreinish · 2026-03-19T03:57:29Z

-                props_map = IndexMap::from_iter([(Qargs::Global, None)]);
-            }
+        let properties = match instruction {
+            TargetOperation::Variadic(_) => IndexMap::from_iter([(Qargs::Global, None)]),


I think this would work and be a bit more concise:

Suggested change

TargetOperation::Variadic(_) => IndexMap::from_iter([(Qargs::Global, None)]),

TargetOperation::Variadic(_) => indexmap!{Qargs::Global => None},

see: https://docs.rs/indexmap/latest/indexmap/macro.indexmap.html

I tried doing this but it doesn't seem to work too well currently because of two reasons:

indexmap! {} uses the default hasher and here we require usage of ahash::RandomState.

indexmap_with_default! {} wraps the custom hasher inside of a BuildHasherDefault which doesn't evaluate to the same types here and results in the compiler yelling about not evaluating the match arm correctly.
expected IndexMap<{unknown}, {unknown}, BuildHasherDefault<RandomState>>, found IndexMap<Qargs, Option<InstructionProperties>, RandomState>

mtreinish · 2026-03-19T04:01:19Z

                        if qarg_slice.len() != instruction.num_qubits() as usize {
                            return Err(TargetError::QargsMismatch {
-                                instruction: name,
+                                instruction: name.to_string(),


I know I'm the one always going on about using &str in rust interfaces. But I wonder if in this case passing an owned string to this inner method might be a better choice. Then we could avoid this copy. If you have the python method take an owned string, it shouldn't need a clone() anymore since everything else in the python add_instruction() should be by reference after this change. So if you took an owned string in from python and moved it here we'd avoid a clone() or to_string() I think. For a rust/c public/outward facing interface we can still work with &str if that's more ergonomic and do the copy before calling the inner method.

I think it would make sense since, after all we are keeping the string inside the mapping, so I agree with this assessment. That said, it might be too late to include that in this PR as it would change the input of one of the most important methods in this struct. Perhaps we might do this for a future PR I have planned.

I'm not sure I follow, this PR hasn't merged yet how is it too late? I think most of what I was proposing was just undoing changes that this is making and removing clones/to_string calls that aren't needed with the new internal data structure.

If you're thinking about the 2.40rc1 scheduled for today I don't think we should rush to include this in the release and should just do it for 2.5.0. Even if it were ready a change to a core data model like this is not something you want to merge the day of a release, it's too risky since there is no margin to catch mistakes before release.

Oh, I'm sorry. This PR has been open for a bit and I had forgotten about introducing the changes here. Yes I can revert it now.

mtreinish · 2026-03-19T04:03:31Z


-    /// Retrieve all the gate names in the Target
-    // TODO: Remove once `Target` is being consumed.
-    #[allow(dead_code)]


Something is using it now?

Nothing is using it currently but I wanted to make sure that if it weren't it wouldn't be supressed by this flag.

Cryoris

Matt superseded the remaining comments I had, so only 1 is left 🤷🏻‍♂️

- Replace ambiguous type declarations with more explicit types. - `PropsMap` to `TargetIndexMap<K,V>` == `IndexMap<K,V,RandomState>`. - Make `TargetProperties` private. - Remove `_raw_operation_from_name`. - Use `IndexMap::with_capacity_and_hasher` during deserialization. - Revert overusage of `&str`.

- In cases where the alias of `IndexMap<String, TargetProperties, ahash::RandomState>` looked too long, I matched it to the `Index` trait whose return `Output` value is already of said type.

Initial: Clean up target mappings The following commits remove the redundant mappings `gate_name_map` and `angle_bounds` in favor of one mapping with a struct that contains all of that data. This results in a much lighter `Target` going from 504 (0x1F8) bytes to 288 bytes. The new struct called `TargetProperties` is currently internal-only. And contains three fields. - `properties` containing the mapping of the qargs and instruction properties (originally the `gate_map`). - `instruction` containing the `TargetOperation` (originally the `gate_name_map`). - `angle_bounds`: Optionally containing the angle bounds for the ``Target`` (originally `angle_bounds`.) This is the first effort to more efficiently use the ``Target`` in Rust with upcoming efforts focusing on its behavior on Python. Fix: Remove clippy unused

raynelfss requested a review from a team as a code owner November 17, 2025 20:00

raynelfss added Changelog: None Do not include in the GitHub Release changelog. Rust This PR or issue is related to Rust code in the repository mod: transpiler Issues and PRs related to Transpiler labels Nov 17, 2025

raynelfss added this to the 2.3.0 milestone Nov 17, 2025

github-project-automation Bot added this to Qiskit 2.3 Nov 17, 2025

github-project-automation Bot moved this to Ready in Qiskit 2.3 Nov 17, 2025

Cryoris removed this from Qiskit 2.3 Dec 12, 2025

Cryoris modified the milestones: 2.3.0, 2.4.0 Dec 12, 2025

github-project-automation Bot added this to Qiskit 2.4 Dec 12, 2025

github-project-automation Bot moved this to Ready in Qiskit 2.4 Dec 12, 2025

raynelfss added 2 commits January 14, 2026 14:47

Fix: Remove clippy unused

b74ea21

raynelfss force-pushed the target-lighter branch from 33af41d to b74ea21 Compare January 14, 2026 19:47

raynelfss mentioned this pull request Jan 26, 2026

Make Target use interners to store gate names #14040

Closed

Merge remote-tracking branch 'upstream/main' into target-lighter

c06c95f

raynelfss changed the title ~~Clean up Target mappings~~ Consolidate Target mappings Mar 18, 2026

mtreinish reviewed Mar 19, 2026

View reviewed changes

mtreinish self-assigned this Mar 19, 2026

Cryoris reviewed Mar 19, 2026

View reviewed changes

Comment thread crates/transpiler/src/target/mod.rs Outdated

jakelishman modified the milestones: 2.4.0, 2.5.0 Mar 19, 2026

raynelfss removed this from Qiskit 2.4 Mar 19, 2026

raynelfss added this to Qiskit 2.5 Mar 19, 2026

github-project-automation Bot moved this to Ready in Qiskit 2.5 Mar 19, 2026

raynelfss mentioned this pull request Mar 23, 2026

Clean up Target's core data model. #14267

Open

2 tasks

raynelfss mentioned this pull request Mar 23, 2026

Split Target / PyTarget #15857

Draft

5 tasks

raynelfss and others added 2 commits April 13, 2026 11:06

Refactor: Remove type alias.

232bebb

- In cases where the alias of `IndexMap<String, TargetProperties, ahash::RandomState>` looked too long, I matched it to the `Index` trait whose return `Output` value is already of said type.

Merge branch 'main' into target-lighter

2052700

raynelfss requested a review from Cryoris April 30, 2026 15:22

		global_operations: HashMap<u32, HashSet<String>>,
		qarg_gate_map: HashMap<Qargs, HashSet<String>>,

	TargetOperation::Variadic(_) => IndexMap::from_iter([(Qargs::Global, None)]),
	TargetOperation::Variadic(_) => indexmap!{Qargs::Global => None},

Conversation

raynelfss commented Nov 17, 2025

Summary

Details and comments

Uh oh!

qiskit-bot commented Nov 17, 2025

Uh oh!

coveralls commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 21007639926

Details

💛 - Coveralls

Uh oh!

mtreinish 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!

Uh oh!

Uh oh!

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!

raynelfss Mar 19, 2026 • 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!

Cryoris left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

coveralls commented Nov 17, 2025 •

edited

Loading

raynelfss Mar 19, 2026 •

edited

Loading