Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

remove qobj backend run #32

Merged
merged 3 commits into from
Aug 17, 2021
Merged

Conversation

kt474
Copy link
Member

@kt474 kt474 commented Aug 16, 2021

Summary

Passing a qobj to backend.run is deprecated - removing from these test cases

Details and comments

@jyu00
Copy link
Collaborator

jyu00 commented Aug 16, 2021

I have to disagree with this removal. Qobj is how people submit a job pretty much since the beginning, and chances are a lot of people still do it that way, even though it's deprecated. I think it'd be useful to keep some Qobj tests, especially those related to modifying Qobj header, until we're ready to remove Qobj entirely.

@kt474
Copy link
Member Author

kt474 commented Aug 16, 2021

@jyu00 does the same go for #23?

@rathishcholarajan
Copy link
Member

@jyu00 Tests are failing due to the Deprecation Warning. Any suggestions on how we can get around this if we don't want to remove the tests? Also qobj was deprecated in 0.12 and it's time to remove it according to our deprecation policy. Should we make an exception here and how much more longer should we supporting this?

@jyu00
Copy link
Collaborator

jyu00 commented Aug 16, 2021

does the same go for #23?

@kt474 the rng code should be updated, but it lives in the qiskit_rng repo . It's a bit tricky because it already not use Qobj for jobs submitted to IBMQBackend, but it looks for the IBMQBackend class in ibmq-provider, not qiskit_ibm. Perhaps we can make this change after qiskit_ibm is released.

Any suggestions on how we can get around this if we don't want to remove the tests?

@rathishcholarajan Deprecation warnings are converted to errors in qiskit-terra's QiskitTestCase, which itself also mark some classes as "deprecation allowed". We don't use QiskitTestCase directly as it's intended for qiskit-terra only (per class doc). However, we do use ProviderTestCase, which inherits QiskitTestCase. ProviderTestCase probably shouldn't inherit QiskitTestCase since the former is meant for all providers whereas the latter for just terra. You can open an issue in qiskit-terra about this. In the meantime we can just not use ProviderTestCase. Although we should probably implement something similar that promotes deprecation warnings to errors, with our own white list.

Also qobj was deprecated in 0.12 and it's time to remove it according to our deprecation policy.

Our deprecation policy says 3 months is the minimum. It also says that for significant features, the period should be at least double that. Given the impact of Qobj removal, I'd say 9-12 months, or until QASM3 is fully supported (so we can encourage people to move to using QASM3).

@mtreinish
Copy link
Member

@rathishcholarajan Deprecation warnings are converted to errors in qiskit-terra's QiskitTestCase, which itself also mark some classes as "deprecation allowed". We don't use QiskitTestCase directly as it's intended for qiskit-terra only (per class doc). However, we do use ProviderTestCase, which inherits QiskitTestCase. ProviderTestCase probably shouldn't inherit QiskitTestCase since the former is meant for all providers whereas the latter for just terra. You can open an issue in qiskit-terra about this. In the meantime we can just not use ProviderTestCase. Although we should probably implement something similar that promotes deprecation warnings to errors, with our own white list.

I would say create your own base test class in this package and don't use terra's at all. I want to get rid of this shared test class antipattern as it only causes headaches like this, see Qiskit/qiskit#6862. Test code should be self contained, explicit and as flat as possible because you want it to be easy to debug and figure out what it's doing when something fails. Having to look at a parent test class definition in an upstream dependency to figure out how a test works is just going to continually cause issues.

@mtreinish
Copy link
Member

Our deprecation policy says 3 months is the minimum. It also says that for significant features, the period should be at least double that. Given the impact of Qobj removal, I'd say 9-12 months, or until QASM3 is fully supported (so we can encourage people to move to using QASM3).

While I normally support being conservative with the deprecation policy and opting for longer deprecations than shorter ones I don't think it applies here in this case. The qiskit-ibm package is new and already contains a ton of breaking api changes nobody is just going to be able to update a couple of imports in existing code and just use this package (heck not exporting it globally off of qiskit.IBMQ will probably be a breaking change for 90% of the users) I would say we should just remove all the deprecated functionality from qiskit-ibmq-provider before the initial release. If a user was calling backend.run(qobj) on qiskit-ibmq-provider that will continue to be supported until we EoL that package since I agree with the general sentiment on the deprecation and think it should probably keep working until we stop supporting qiskit-ibmq-provider. But if they move to qiskit-ibm which is essentially a new provider we should just start with a strict BackendV1 and move forward. from there.

@jyu00
Copy link
Collaborator

jyu00 commented Aug 16, 2021

I would say we should just remove all the deprecated functionality from qiskit-ibmq-provider before the initial release.

I was debating about this. On the one hand this is our opportunity to make all kinds of breaking changes. On the other I worry that users would not want to move to qiskit_ibm if they have to make a lot of changes instead of simple find/replace. But maybe the latter isn't really an issue because we'll eventually force their hands by archiving qiskit-ibmq-provider lol

@rathishcholarajan
Copy link
Member

Our deprecation policy says 3 months is the minimum. It also says that for significant features, the period should be at least double that. Given the impact of Qobj removal, I'd say 9-12 months, or until QASM3 is fully supported (so we can encourage people to move to using QASM3).

While I normally support being conservative with the deprecation policy and opting for longer deprecations than shorter ones I don't think it applies here in this case. The qiskit-ibm package is new and already contains a ton of breaking api changes nobody is just going to be able to update a couple of imports in existing code and just use this package (heck not exporting it globally off of qiskit.IBMQ will probably be a breaking change for 90% of the users) I would say we should just remove all the deprecated functionality from qiskit-ibmq-provider before the initial release. If a user was calling backend.run(qobj) on qiskit-ibmq-provider that will continue to be supported until we EoL that package since I agree with the general sentiment on the deprecation and think it should probably keep working until we stop supporting qiskit-ibmq-provider. But if they move to qiskit-ibm which is essentially a new provider we should just start with a strict BackendV1 and move forward. from there.

Thanks @mtreinish and @jyu00! I agree that we should remove deprecated functionality before initial release of qiskit-ibm. So will go ahead and approve this PR.

Copy link
Member

@rathishcholarajan rathishcholarajan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kt474 Thanks!

@rathishcholarajan rathishcholarajan merged commit a406a0e into Qiskit:main Aug 17, 2021
@rathishcholarajan rathishcholarajan added this to the 0.1 milestone Aug 20, 2021
@kt474 kt474 deleted the qobj-deprecation branch April 21, 2022 17:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants