Update Torch tensor.to_numpy dispatch to stay compatible with torch<=1.9.0
#1825
Conversation
|
Hello. You may have forgotten to update the changelog!
|
antalszava
changed the title
tensor.to_numpy dispatch to stay compatible with torch<=1.9.0
Codecov Report
@@ Coverage Diff @@
## master #1825 +/- ##
=======================================
Coverage 98.96% 98.96%
=======================================
Files 218 218
Lines 16368 16368
=======================================
Hits 16199 16199
Misses 169 169
Continue to review full report at Codecov.
|
| @@ -269,7 +269,7 @@ def _scatter_element_add_tf(tensor, index, value): | |||
|
|
|||
|
|
|||
| def _to_numpy_torch(x): | |||
| if x.is_conj(): | |||
| if getattr(x, "is_conj", False) and x.is_conj(): | |||
.github/workflows/tests.yml
Outdated
| - {python-version: 3.7, interfaces: ['torch'], interface_version: 1.9.0+cpu} | ||
| - {python-version: 3.7, interfaces: ['torch'], interface_version: 1.10.0+cpu} | ||
| - {python-version: 3.8, interfaces: ['torch'], interface_version: 1.10.0+cpu} | ||
|
|
||
| - {python-version: 3.7, interfaces: ['tf'], interface_version: 2.5} | ||
| - {python-version: 3.7, interfaces: ['tf'], interface_version: 2.6} | ||
| - {python-version: 3.8, interfaces: ['tf'], interface_version: 2.6} |
There was a problem hiding this comment.
I agree it’s good to test as many configurations as we can, but I would like to balance it against ensuring the CI does not take too long for contributors. I also want to lesson the maintenance load on the development team.
What do you think about ensuring PennyLane compatibility with only the latest 2.X (TF) and 1.X (PyTorch) versions via the CI suite?
-
With semantic versioning, TensorFlow and PyTorch should not be introducing breaking changes in minor release versions (Torch complex number support is the one exception, since complex support was not available in 1.0)
-
The onus on the developers for maintenance then shifts to ensuring that the QML demos work with the latest releases of TF and Torch, which I think is more important for users than ensuring PL works for older versions.
There was a problem hiding this comment.
Of course, we can re-evaluate when there are new major version releases, but with semantic versioning, I think it makes sense to guarantee support for only the latest minor versions.
Note: this doesn’t mean we actively remove support for older minor versions, just that the CI only verifies the PL test suite against the latest minor versions.
There was a problem hiding this comment.
I think this makes sense and also was wondering if we should have these many checks here.
What if we had a weekly/on dispatch check that runs with multiple interface versions? That way we could double-check from time to time if we have an implementation in PennyLane that breaks with older versions of an ML framework.
Having many checks on every PR run might indeed decrease the development momentum.
There was a problem hiding this comment.
I'll revert the changes to this file for now. They were meant to just double-check that the changes in place were effective.
There was a problem hiding this comment.
What if we had a weekly/on dispatch check that runs with multiple interface versions? That way we could double-check from time to time if we have an implementation in PennyLane that breaks with older versions of an ML framework.
Makes more sense I think than having it in the main CI!
Although, we should definitely codify our dependency support; with semantic versioning, the onus should not be on PennyLane to test against older minor versions of dependencies.
is_conjattribute for Torch tensors was introduced in version 1.10.0. Therefore, code that usestensor.to_numpywill fail (e.g., see a related demo run):