search

CQCL / pytket-qiskit

pytket-qiskit, extensions for pytket quantum SDK
Apache License 2.0
15 stars 13 forks source link

Question regarding version requirements #351

Closed nquetschlich closed 2 months ago

nquetschlich commented 3 months ago

Hi there, I have noticed that the qiskit and tket version requirements are in the format of 

"pytket ~= 1.x",
"qiskit ~= 1.y",

This has the effect, that for each new release of those two packages of version 1.x.y are included, e.g., when pytket was upgrade from 1.28.0 to 1.29.0, the requirement of pytket ~= 1.27 automatically included it (since 1.29.0 satisfies ~= 1.27). Would it possible to switch to

"pytket ~= 1.x.0",
"qiskit ~= 1.y.0",

by including the patch version as well?

We are heavily using this package as a dependency and this versioning scheme has led to some unexpected changed behavior and makes versioning not so easy for us.

cqc-alec commented 3 months ago

This versioning scheme was an intentional choice, since in theory pytket 1.y should be backwards-compatible with pytket 1.x when y >= x, but may have bugfixes and improvements, and we don't want to have to make a new release of every extension whenever there is a new pytket release. (Similarly for qiskit.) However, I do realize that it makes reproducibility more difficult, and there are occasionally cases where behaviour changes in ways that we don't realize anyone depends on, or bugs are introduced in new versions. Would it work for you to take a snapshot of installed versions of all python packages as a way to ensure reproducibility?

nquetschlich commented 3 months ago

Thanks for you quick response, @cqc-alec. Let me give you a bit more of context why this is a problem for us: In our project MQT Bench (https://github.com/cda-tum/mqt-bench), we follow the Scientific Python Development Guide that suggest to mark warnings as error during the CI triggering pytest (similar to https://learn.scientific-python.org/development/guides/pytest/#configuring-pytest).

The problem is now, that the current version management here automatically pulls the latest pytket(or qiskit) version that might be backwards-compatible, but occasionally introduce new DeprecationWarnings that then lead our CI pipelines to fail. However, since the our dependencies have not changed, it is often hard then to deal with those warnings/errors since we cannot just increase the lower cap of a dependency and have to find a workaround (see, e.g., https://github.com/cda-tum/mqt-bench/pull/344). Furthermore, reproducibility keeps a problem and taking a snapshot of all installed packages would severely limit the compatibility of MQT Bench.

cqc-alec commented 3 months ago

Could you perhaps use a pip constraints file to limit the versions of packages you'd like to install?

burgholzer commented 3 months ago

Hi all,

Let me quickly chime in here from the MQT maintainer side and add a couple of comments.

First of all, I agree that, in principle, if a library truly follows semantic versioning, then version a.b.c should be compatible with any version under the same major version, i.e., a.x.z This is also what specifying a dependency in the form ~=a.b means. It will accept any new patch version and any new minor version.

Now, in practice, and even more so in a young field such as quantum computing, almost no library truly follows semantic versioning. Many developers are very hesitant to increase major versions which leads to breaking changes also being introduced in minor versions (even if these changes are hidden as features). In these cases, just using ~=a.b is simply not safe/recommended.

Please correct me if I am wrong, but pull requests such as https://github.com/CQCL/pytket-qiskit/pull/349 also give the impression that you might be misinterpreting what ~=a.b means. Let's say you are specifying qiskit~=1.0. Then you are directly saying this will work with any qiskit version of the form 1.x.y for any x and y. The fact that the above PR exists, kind of tells me, that that promise was not really true because something needed to be fixed to support qiskit 1.1. What I would suspect that you really intended here was to communicate compatibility with any qiskit version of the form 1.0.y, which is what would be accomplished with qiskit~=1.0.0. Should my suggestions be wrong (which is definitely possible), then I would seriously question the existence of #349, because compatibility with the versions stated in that PR was already communicated by only using ~=a.b for those dependencies. That PR effectively only raises the lower bound for these packages, which indicates an incompatibility with previous versions; contradicting the premise of using ~=a.b in the first place.

Because you stated in one of the comments above that you wouldn't want to release new versions of this plugin for every new release of Qiskit or pytket: you wouldn't have to do that if you were specifying qiskit~=1.1.0, etc. you would only have to create a new release for new minor versions of these tools. Patch version releases would "just work" ™️

I hope these considerations make sense and you'll consider adopting the proposed change.

burgholzer commented 3 months ago

Just some further thoughts:

Based on Qiskit's release cycle alone, this would imply 4 new versions per year. Depending on the pytket release cadence that might be totally reasonable.

In addition, if a new minor version should truly follow SemVer and is fully backward compatible without requiring changes, you can always specify the dependency as e.g. qiskit>=1.0.0,<1.2.0, which would indicate that any patch version of the 1.0 series and the 1.1 series would work.

This is also what dependabot could do for you automatically. If you specify qiskit~=1.0.0 and qiskit 1.1.0 comes out, then dependabot will create a PR that updates the dependency to qiskit>=1.0.0,<1.2.0. If this new minor version should break something, you would immediately notice in the corresponding dependabot PR and could adjust the supported version range. Otherwise, you'd merge the dependabot PR and create a new patch release in this repository to extend the compatibility to the new minor release of the dependency.

cqc-melf commented 3 months ago

Hi all,

Please correct me if I am wrong, but pull requests such as #349 also give the impression that you might be misinterpreting what ~=a.b means. Let's say you are specifying qiskit~=1.0. Then you are directly saying this will work with any qiskit version of the form 1.x.y for any x and y. The fact that the above PR exists, kind of tells me, that that promise was not really true because something needed to be fixed to support qiskit 1.1.

Just to clarify the changes in the PR you mentioned: The idea is that everything before this PR should have worked with qiskit 1.0 to < 2.0. The changes in the other dependencies required some restrictions to make sure everything is working as expected. So I don't see a problem here for this situation? The promise after this PR is again that it should work with qiskit 1.1 to < 2.0.

cqc-melf commented 3 months ago

In addition to the points raised by @cqc-alec we had issues in the past with to tight version making it impossible to install packages together in the same env.

Can you maybe give some more details why it is the best place to tighten the version here in pytket-qiskit and why this can't be on your side?

burgholzer commented 3 months ago

Just to clarify the changes in the PR you mentioned: The idea is that everything before this PR should have worked with qiskit 1.0 to < 2.0. The changes in the other dependencies required some restrictions to make sure everything is working as expected. So I don't see a problem here for this situation? The promise after this PR is again that it should work with qiskit 1.1 to < 2.0.

That PR updates the version bounds for several packages:

(I'll just ignore the upper cap for numpy 2.0 for now as it is not related to the issue at hand) Note that out of the dependencies above, only the qiskit-ibm-runtime change was not covered by the previous version ranges and it is the only change, where also the upper cap has been effectively increased.

The fact that these dependency lower bounds were upgraded (based on changes in these dependencies, as you said) indicates to me that the premise of the previous version of the code being compatible with

was not fully true.

In addition to the points raised by @cqc-alec we had issues in the past with to tight version making it impossible to install packages together in the same env.

Can you maybe give some more details why it is the best place to tighten the version here in pytket-qiskit and why this can't be on your side?

Supported version ranges should always be as large as possible. And I can only say that we faced similar problems with too tight version ranges in the past; so I know the feeling. Upper caps, which are (extensively) used in this package, should in general be avoided all together if possible. I can only recommend reading through: https://iscinumpy.dev/post/bound-version-constraints. Using ~= in whatever form directly creates (rather tight) upper bounds that might break if a library "breaks its promise of following SemVer".

I gave this quite some thought now as this is definitely not an easy thing to decide on in general. Based on these thoughts and also on re-reading the linked article from above, I'd actually not suggest to limit the dependency versions to, e.g., qiskit~=1.1.0. However, I'd also not recommend to set them to, e.g., qiskit~=1.1, because, as demonstrated by the example above, these promises of semantic versioning rarely hold and the upper caps introduced in the process create more harm than good. What I would propose is to effectively drop the upper caps altogether here, i.e., to simply specify appropriate lower bounds for all dependencies. For example, based on #349, these could be:

Should a problem be discovered with one of the versions, such as for example a qiskit-aer update being necessary to support qiskit 1.1, then the lower bound should be raised here. This effectively allows for maximum compatibility in the future and only requires updates to this package if problems are discovered. (what I can also recommend here is to turn on the "warnings=error" feature of pytest in your CI as it really helps to catch deprecation warnings and other incompatibilities early)

@nquetschlich what this means on our side is that situations might occur, where we need to temporarily upper cap the qiskit or pytket version if any version update breaks one of our libraries or the pytket-qiskit library. This is quite a natural process though and, most of the time, our CI will actually catch errors (such as the new dependency warnings that originally prompted this issue) way before these actual consequences of these hit users of our tools. And, if we really would want to control the supported versions of pytket and qiskit in our tools, we could upper cap them and maintain these upper caps using dependabot.

cqc-alec commented 2 months ago

I gave this quite some thought now as this is definitely not an easy thing to decide on in general. Based on these thoughts and also on re-reading the linked article from above, I'd actually not suggest to limit the dependency versions to, e.g., qiskit~=1.1.0. However, I'd also not recommend to set them to, e.g., qiskit~=1.1, because, as demonstrated by the example above, these promises of semantic versioning rarely hold and the upper caps introduced in the process create more harm than good. What I would propose is to effectively drop the upper caps altogether here, i.e., to simply specify appropriate lower bounds for all dependencies. For example, based on #349, these could be:

* `qiskit>=1.1`

* `pytket>=1.29`

* `qiskit-aer>=0.14.2`

* `qiskit-ibm-runtime>=0.24.1`

The general rule we follow is to require ~=x.y when x >=1 and ~=0.y.z; that is we assume 0.y2 may have breaking changes compared to 0.y1 when y2 > y1, even though I think strictly semver doesn't allow this (icbw).

We don't want to allow qiskit 2.0 (or pytket 2.0), since those will almost certainly break stuff, whereas new qiskit 1.x or pytket 1.x releases shouldn't (even though they occasionally do, but we detect those cases on CI as you say).

We'll update the pytket lower bound when a new pytket version is released that fixes bugs or adds functionality relevant to the extension. I accept that we may be a little overzealous in updating lower bounds where we're not aware of any need to do so. It does make testing easier, however, since we only need to test with the latest version of a given dependency rather than needing to test with previous versions as well.

burgholzer commented 2 months ago

The general rule we follow is to require ~=x.y when x >=1 and ~=0.y.z; that is we assume 0.y2 may have breaking changes compared to 0.y1 when y2 > y1, even though I think strictly semver doesn't allow this (icbw).

We don't want to allow qiskit 2.0 (or pytket 2.0), since those will almost certainly break stuff, whereas new qiskit 1.x or pytket 1.x releases shouldn't (even though they occasionally do, but we detect those cases on CI as you say).

We'll update the pytket lower bound when a new pytket version is released that fixes bugs or adds functionality relevant to the extension. I accept that we may be a little overzealous in updating lower bounds where we're not aware of any need to do so. It does make testing easier, however, since we only need to test with the latest version of a given dependency rather than needing to test with previous versions as well.

I really really recommend reading through the article linked above (https://iscinumpy.dev/post/bound-version-constraints) for why translating these assumptions into upper version caps is hardly ever a good idea. I'll try to re-collect a couple thoughts here.

Updating lower bounds aggressively is not really an issue. Typically, it's rather a good thing to work with modern versions of the respective tools as it also encourages others to not stick to very old versions. However, it is not good to combine high lower bounds with tight upper bounds (as, e.g., for the 0.x.y) libraries.

In general, a package with underconstrained dependencies is always fixable. Users (such as us) can always introduce an upper cap for one of the dependencies once we notice something is going on. While this is a small nuisance, it's fixable. However, an overconstrained system is unfixable as it is simply impossible in the Python ecosystem to remove an upper cap in a library you depend on.

So while your assumption might be right that Qiskit 2.0 and pytket 2.0 or qiskit-aer 0.15 might break things, they might as well not. And if they do not, then you overconstrained your package unnecessarily. If things really break, you will notice in your CI. Oftentimes before that even hits users. Qiskit, e.g., does release candidates before major releases; maybe even minor releases. So you can test these before their release. If they should break things, you can temporarily introduce the upper cap while you work on fixing things.

Again, I am just repeating some arguments from the article above.

cqc-alec commented 2 months ago

Thanks, this is an interesting article and I will give it some thought. Your suggestion (to remove upper caps) is the opposite of the original suggestion in this issue (which was to introduce stricter upper caps): so, it's a complicated problem to which there may be no really good solution.

I'm almost persuaded that we should remove the upper caps altogether, and only add constraints where we know they are necessary. The major downside would be that we'd only know this after the event -- after an update came along that broke something. perhaps for a lot of users.

burgholzer commented 2 months ago

Your suggestion (to remove upper caps) is the opposite of the original suggestion in this issue (which was to introduce stricter upper caps): so, it's a complicated problem to which there may be no really good solution.

Fully agree. It's a difficult question with no "one-fits-all" answer. After posting my initial suggestion (of even tighter bounds), I couldn't help but remember the article from above and how convincingly it argued for explicitly not using upper caps heavily (especially for libraries).

The major downside would be that we'd only know this after the event -- after an update came along that broke something. perhaps for a lot of users.

While that can be a nuisance to some users, it is always fixable on the user side by constraining the version if they are affected and until this library here is fixed. From experience, I can tell that we did that several times over at our MQT Bench or MQT Predictor repositories because some Qiskit update broke us.

Adding upper caps puts quite a bit of pressure on the maintainers of the library here to quickly update the version bounds when new versions of its dependencies come out. Because if not updated quickly, users that want to use this library are still limited to old version of its dependencies (e.g., qiskit and/or pytket) until maintainers officially declare this package compatible and release a new version.

I believe the benefits of dropping the upper caps significantly outweigh the potential drawbacks.

cqc-alec commented 2 months ago

Because if not updated quickly, users that want to use this library are still limited to old version of its dependencies (e.g., qiskit and/or pytket) until maintainers officially declare this package compatible and release a new version.

This is true. OK, happy to give this a try.