Do not raise deprecation warnings for internal uses of dag.duration and dag.unit #14133

ElePT · 2025-03-31T09:47:01Z

Summary

dag.duration and dag.unit raise a deprecation warning from Rust which I believe doesn't follow the right stack level, because every time dag_to_circuit is called, these deprecation warnings are raised to the user as if they were user-actionable (which they are not). This happens not only for direct calls to dag_to_circuit (and a couple others), but also every time dag_to_circuit is used internally, flooding test runs and confusing both users and downstream package devs.

We don't see them because we have a blanket filter in our unit test configuration, but now that the deprecation warnings are going to stay until 3.0, I think that at least a filter would improve the user experience of dag_to_circuit.

Details and comments

…ternally Filter Rust deprecation warning when calling dag.duration/dag.unit internally

qiskit-bot · 2025-03-31T09:47:05Z

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

@Qiskit/terra-core

coveralls · 2025-03-31T10:11:22Z

Pull Request Test Coverage Report for Build 14173060988

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

34 of 68 (50.0%) changed or added relevant lines in 9 files are covered.
75 unchanged lines in 9 files lost coverage.
Overall coverage decreased (-0.1%) to 88.001%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/dag_circuit.rs	10	44	22.73%

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/unitary_synthesis.rs	1	94.79%
crates/qasm2/src/expr.rs	1	94.23%
qiskit/circuit/quantumcircuit.py	2	93.57%
qiskit/result/models.py	4	92.77%
crates/qasm2/src/lex.rs	8	91.98%
crates/circuit/src/dag_circuit.rs	11	85.57%
qiskit/circuit/duration.py	11	44.0%
qiskit/result/result.py	13	85.22%
crates/qasm2/src/parse.rs	24	96.22%

Totals
Change from base Build 14138609516:	-0.1%
Covered Lines:	72741
Relevant Lines:	82659

💛 - Coveralls

mtreinish

Instead of filtering warnings which is kind of imprecise shouldn't we just adjust the access patterns to not trigger a warning? It's pretty simple in this case by just adding and using _duration and _unit

mtreinish · 2025-03-31T10:08:59Z

qiskit/converters/dag_to_circuit.py

@@ -75,6 +76,19 @@ def dag_to_circuit(dag, copy_operations=True):
    circuit.metadata = dag.metadata or {}
    circuit._data = circuit_data

-    circuit._duration = dag.duration
-    circuit._unit = dag.unit
+    with warnings.catch_warnings():


Instead of filtering the warning why not just use the internal attribute which doesn't have the warning?

mtreinish · 2025-03-31T10:22:08Z

which I believe doesn't follow the right stack level

We explicitly set the stack level to because we're just calling warnings.warn() via Python if python isn't counting the rust space method in the stack we can adjust that.

We don't see them because we have a blanket filter in our unit test configuration, but now that the deprecation warnings are going to stay until 3.0, I think that at least a filter would improve the user experience of dag_to_circuit.

If the filter is masking bugs we should remove it.

jakelishman · 2025-03-31T10:29:52Z

if python isn't counting the rust space method in the stack

It doesn't because the Rust-space call is invisible to Python:

qiskit/crates/circuit/src/circuit_instruction.rs

Lines 687 to 711 in 82b2da2

    
           /// Issue a Python `DeprecationWarning` about using the legacy tuple-like interface to 
        
           /// `CircuitInstruction`. 
        
           /// 
        
           /// Beware the `stacklevel` here doesn't work quite the same way as it does in Python as Rust-space 
        
           /// calls are completely transparent to Python. 
        
           #[inline] 
        
           fn warn_on_legacy_circuit_instruction_iteration(py: Python) -> PyResult<()> { 
        
               WARNINGS_WARN 
        
                   .get_bound(py) 
        
                   .call1(( 
        
                       intern!( 
        
                           py, 
        
                           concat!( 
        
                               "Treating CircuitInstruction as an iterable is deprecated legacy behavior", 
        
                               " since Qiskit 1.2, and will be removed in Qiskit 3.0.", 
        
                               " Instead, use the `operation`, `qubits` and `clbits` named attributes." 
        
                           ) 
        
                       ), 
        
                       py.get_type::<PyDeprecationWarning>(), 
        
                       // Stack level.  Compared to Python-space calls to `warn`, this is unusually low 
        
                       // beacuse all our internal call structure is now Rust-space and invisible to Python. 
        
                       1, 
        
                   )) 
        
                   .map(|_| ()) 
        
           }

.

mtreinish · 2025-03-31T11:04:00Z

Ah, then the fix here is simply changing the 2 to a 1 on the offending warn() calls.

* Refactor access patterns: add dag._duration and dag._unit * Refactor internal uses of dag.duration and dag.unit to rely on internal methods * Remove blanket warning filters from unit test config On top of these: * Extend deprecation warnings to dag.duration and dag.unit setters (previously only in getters) * Clean up unit test config from old filters

ElePT · 2025-03-31T11:34:46Z

Heh, saw the last suggestion too late. But it's probably good not to have internal calls to deprecated methods anyways, and I also added a deprecation warning on the setter that I think we were missing (correct me if I'm wrong).

jakelishman · 2025-03-31T12:28:08Z

Oh yeah, I'm not a fan of using filterwarnings inside library code, even within catch_warnings - I'd rather use the privileged internal methods to support the deprecated methods, since that's our prerogative as the library authors. I was just commenting about how to fix the stack level (which we should do too, as you did).

mtreinish

This LGTM now thanks for updating this.

mtreinish · 2025-03-31T13:35:48Z

crates/circuit/src/dag_circuit.rs

@@ -521,12 +519,49 @@ impl DAGCircuit {
                )
            ),
            py.get_type::<PyDeprecationWarning>(),
-            2,
+            1,
        ))?;
        Ok(self.duration.as_ref().map(|x| x.clone_ref(py)))


We probably could have done this to deduplicate the code a little bit:

Suggested change

Ok(self.duration.as_ref().map(|x| x.clone_ref(py)))

self.get_internal_duration(py)

but it's not a big deal. Not worth blocking over.

I had to change a small detail to fix lint so I can also fix this

…al attributes

…nd dag.unit (#14133) * Filter Rust deprecation warning when calling dag.duration/dag.unit internally Filter Rust deprecation warning when calling dag.duration/dag.unit internally * Apply Matt's suggestions: * Refactor access patterns: add dag._duration and dag._unit * Refactor internal uses of dag.duration and dag.unit to rely on internal methods * Remove blanket warning filters from unit test config On top of these: * Extend deprecation warnings to dag.duration and dag.unit setters (previously only in getters) * Clean up unit test config from old filters * Change stacklevel * Fix circuit_to_dag * Handle failing tests * _DAGDependencyV2 is private and not a DAGCircuit, so don't use internal attributes * Reduce duplication in getters and setters (cherry picked from commit d0aa100)

…nd dag.unit (#14133) (#14140) * Filter Rust deprecation warning when calling dag.duration/dag.unit internally Filter Rust deprecation warning when calling dag.duration/dag.unit internally * Apply Matt's suggestions: * Refactor access patterns: add dag._duration and dag._unit * Refactor internal uses of dag.duration and dag.unit to rely on internal methods * Remove blanket warning filters from unit test config On top of these: * Extend deprecation warnings to dag.duration and dag.unit setters (previously only in getters) * Clean up unit test config from old filters * Change stacklevel * Fix circuit_to_dag * Handle failing tests * _DAGDependencyV2 is private and not a DAGCircuit, so don't use internal attributes * Reduce duplication in getters and setters (cherry picked from commit d0aa100) Co-authored-by: Elena Peña Tapia <[email protected]>

ElePT · 2025-04-01T09:40:34Z

@Mergifyio backport stable/1.4

mergify · 2025-04-01T09:40:38Z

backport stable/1.4

✅ Backports have been created

#14147 Do not raise deprecation warnings for internal uses of dag.duration and dag.unit (backport #14133) has been created for branch stable/1.4 but encountered conflicts

…nd dag.unit (#14133) * Filter Rust deprecation warning when calling dag.duration/dag.unit internally Filter Rust deprecation warning when calling dag.duration/dag.unit internally * Apply Matt's suggestions: * Refactor access patterns: add dag._duration and dag._unit * Refactor internal uses of dag.duration and dag.unit to rely on internal methods * Remove blanket warning filters from unit test config On top of these: * Extend deprecation warnings to dag.duration and dag.unit setters (previously only in getters) * Clean up unit test config from old filters * Change stacklevel * Fix circuit_to_dag * Handle failing tests * _DAGDependencyV2 is private and not a DAGCircuit, so don't use internal attributes * Reduce duplication in getters and setters (cherry picked from commit d0aa100) # Conflicts: # crates/circuit/src/dag_circuit.rs # qiskit/transpiler/passes/scheduling/padding/base_padding.py # test/python/circuit/test_scheduled_circuit.py # test/python/compiler/test_transpiler.py # test/utils/base.py

…nd dag.unit (backport #14133) (#14147) * Do not raise deprecation warnings for internal uses of dag.duration and dag.unit (#14133) * Filter Rust deprecation warning when calling dag.duration/dag.unit internally Filter Rust deprecation warning when calling dag.duration/dag.unit internally * Apply Matt's suggestions: * Refactor access patterns: add dag._duration and dag._unit * Refactor internal uses of dag.duration and dag.unit to rely on internal methods * Remove blanket warning filters from unit test config On top of these: * Extend deprecation warnings to dag.duration and dag.unit setters (previously only in getters) * Clean up unit test config from old filters * Change stacklevel * Fix circuit_to_dag * Handle failing tests * _DAGDependencyV2 is private and not a DAGCircuit, so don't use internal attributes * Reduce duplication in getters and setters (cherry picked from commit d0aa100) # Conflicts: # crates/circuit/src/dag_circuit.rs # qiskit/transpiler/passes/scheduling/padding/base_padding.py # test/python/circuit/test_scheduled_circuit.py # test/python/compiler/test_transpiler.py # test/utils/base.py * Fix merge conflicts * Address failing tests * More fixes * More fixes * Last try --------- Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: Elena Peña Tapia <[email protected]>

Filter Rust deprecation warning when calling dag.duration/dag.unit in…

7fad4b1

…ternally Filter Rust deprecation warning when calling dag.duration/dag.unit internally

ElePT requested a review from a team as a code owner March 31, 2025 09:47

ElePT added this to the 2.0.0 milestone Mar 31, 2025

ElePT added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: None Do not include in changelog labels Mar 31, 2025

1ucian0 previously approved these changes Mar 31, 2025

View reviewed changes

1ucian0 enabled auto-merge March 31, 2025 10:04

mtreinish disabled auto-merge March 31, 2025 10:07

mtreinish requested changes Mar 31, 2025

View reviewed changes

ElePT dismissed 1ucian0’s stale review via ecb5e54 March 31, 2025 11:31

ElePT requested a review from nonhermitian as a code owner March 31, 2025 11:31

ElePT added 2 commits March 31, 2025 13:35

Change stacklevel

5ff21d2

Fix circuit_to_dag

55c27fb

Handle failing tests

cce30af

mtreinish previously approved these changes Mar 31, 2025

View reviewed changes

_DAGDependencyV2 is private and not a DAGCircuit, so don't use intern…

5be004d

…al attributes

ElePT dismissed mtreinish’s stale review via 5be004d March 31, 2025 13:37

Reduce duplication in getters and setters

320a8e3

mtreinish approved these changes Mar 31, 2025

View reviewed changes

mtreinish enabled auto-merge March 31, 2025 13:50

mtreinish added this pull request to the merge queue Mar 31, 2025

Merged via the queue into Qiskit:main with commit d0aa100 Mar 31, 2025
24 checks passed

mergify bot mentioned this pull request Mar 31, 2025

Do not raise deprecation warnings for internal uses of dag.duration and dag.unit (backport #14133) #14140

Merged

mergify bot mentioned this pull request Apr 1, 2025

Do not raise deprecation warnings for internal uses of dag.duration and dag.unit (backport #14133) #14147

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not raise deprecation warnings for internal uses of dag.duration and dag.unit #14133

Do not raise deprecation warnings for internal uses of dag.duration and dag.unit #14133

ElePT commented Mar 31, 2025

qiskit-bot commented Mar 31, 2025

coveralls commented Mar 31, 2025 •

edited

Loading

mtreinish left a comment •

edited

Loading

mtreinish Mar 31, 2025

mtreinish commented Mar 31, 2025

jakelishman commented Mar 31, 2025

mtreinish commented Mar 31, 2025

ElePT commented Mar 31, 2025 •

edited

Loading

jakelishman commented Mar 31, 2025

mtreinish left a comment

mtreinish Mar 31, 2025

ElePT Mar 31, 2025

ElePT commented Apr 1, 2025

mergify bot commented Apr 1, 2025 •

edited

Loading

	Ok(self.duration.as_ref().map(\|x\| x.clone_ref(py)))
	self.get_internal_duration(py)

Do not raise deprecation warnings for internal uses of dag.duration and dag.unit #14133

Do not raise deprecation warnings for internal uses of dag.duration and dag.unit #14133

Conversation

ElePT commented Mar 31, 2025

Summary

Details and comments

qiskit-bot commented Mar 31, 2025

coveralls commented Mar 31, 2025 • edited Loading

Pull Request Test Coverage Report for Build 14173060988

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

mtreinish left a comment • edited Loading

Choose a reason for hiding this comment

mtreinish Mar 31, 2025

Choose a reason for hiding this comment

mtreinish commented Mar 31, 2025

jakelishman commented Mar 31, 2025

mtreinish commented Mar 31, 2025

ElePT commented Mar 31, 2025 • edited Loading

jakelishman commented Mar 31, 2025

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish Mar 31, 2025

Choose a reason for hiding this comment

ElePT Mar 31, 2025

Choose a reason for hiding this comment

ElePT commented Apr 1, 2025

mergify bot commented Apr 1, 2025 • edited Loading

✅ Backports have been created

coveralls commented Mar 31, 2025 •

edited

Loading

mtreinish left a comment •

edited

Loading

ElePT commented Mar 31, 2025 •

edited

Loading

mergify bot commented Apr 1, 2025 •

edited

Loading