Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Python 3.12 #1231

Merged
merged 22 commits into from
Apr 24, 2024
Merged

Support Python 3.12 #1231

merged 22 commits into from
Apr 24, 2024

Conversation

alecandido
Copy link
Member

No description provided.

@alecandido
Copy link
Member Author

@natestemen this branch could work (in principle), and we've been kind of lucky: TensorFlow didn't support py3.12 until 5 days ago, when they published a release candidate with initial support (not yet a stable one).

I wonder whether it's this the triggered your update.

I still have to solve part of the problem with the installation, but the dependencies should now be alright.

@alecandido
Copy link
Member Author

@scarrazza is it acceptable to have a TensorFlow release candidate dep in master?

@alecandido
Copy link
Member Author

Assuming that tests are passing, the plan is the following:

  • merge Qibo py3.12 in master, with Qibojit branch dep
  • update Qibojit to Qibo master
  • merge Qibojit py3.12 in main, with Qibo master dep
  • update Qibo to Qibojit main
  • release Qibo, with Qibojit main (it is only a test dep, so it should be reasonable)
  • update Qibojit to Qibo release
  • release Qibojit
  • update Qibo to Qibojit release

@alecandido
Copy link
Member Author

Tests are failing because hyperopt is a relatively old package (no release since two years) and it implicitly requires setuptools
https://github.com/hyperopt/hyperopt/tags
https://github.com/hyperopt/hyperopt/blob/d8515263a3168ba8f111179d29e74ffb09670610/setup.py#L46-L52
https://github.com/hyperopt/hyperopt/blob/d8515263a3168ba8f111179d29e74ffb09670610/hyperopt/atpe.py#L19

I'll try to circumvent the issue, but I'd recommend replacing it, if possible.

@alecandido alecandido changed the title Python 3.12 support Support Python 3.12 Feb 28, 2024
@alecandido
Copy link
Member Author

Required for unitaryfund/mitiq#2066

@alecandido
Copy link
Member Author

Currently stuck because of TensorFlow installation.

@alecandido
Copy link
Member Author

Tests are passing, even though not doctests. The reason is that if tensorflow is not installed, the related tests are skipped:

qibo/tests/conftest.py

Lines 31 to 41 in aa0fab2

# ignore backends that are not available in the current testing environment
AVAILABLE_BACKENDS = []
MULTIGPU_BACKENDS = []
for backend_name in BACKENDS:
try:
_backend = get_backend(backend_name)
AVAILABLE_BACKENDS.append(backend_name)
if _backend.supports_multigpu: # pragma: no cover
MULTIGPU_BACKENDS.append(backend_name)
except (ModuleNotFoundError, ImportError):
pass

@stavros11
Copy link
Member

stavros11 commented Mar 1, 2024

Tests are passing, even though not doctests. The reason is that if tensorflow is not installed, the related tests are skipped:

Thanks @alecandido. This is expected, I think tests (not doctests) were set up so that they only run on backends that are available, mostly for convenience when running them locally in an environment with a subset of backends installed. However, I do not understand why tensorflow is not installed in the CI, since we still have it in the pyproject?

If tensorflow is the only issue blocking this I would consider disabling the related doctests temporarily and opening an issue until tensorflow is "officially" relased for py3.12. That also depends on what release we would like to include this. If I remember correctly is not for the one coming soon so maybe we could even keep on hold. The main motivation is that it is needed by an external project, though.

@alecandido
Copy link
Member Author

However, I do not understand why tensorflow is not installed in the CI, since we still have it in the pyproject?

This is an issue with the release candidate package, most likely. That's why this PR is on hold, waiting for a more stable support for py3.12 from TensorFlow (or to make TensorFlow optional, e.g. outsourcing to qiboml).

@alecandido
Copy link
Member Author

If tensorflow is the only issue blocking this I would consider disabling the related doctests temporarily and opening an issue until tensorflow is "officially" relased for py3.12. That also depends on what release we would like to include this. If I remember correctly is not for the one coming soon so maybe we could even keep on hold. The main motivation is that it is needed by an external project, though.

It's not too urgent, apparently, let's keep going with qibo-core and the reorganization first.

@natestemen
Copy link

Thank you for so much effort after I pinged y'all! Just want to say it's not super urgent on our side.

@alecandido
Copy link
Member Author

Thank you for so much effort after I pinged y'all! Just want to say it's not super urgent on our side.

Thanks for the update @natestemen!
Indeed, I was initially a bit worried to be blocking mitiq, but now we're all waiting for TF.

I also checked the various common distributions, and no major one is shipping py3.12 as default Python yet.

In any case, we'll work on reducing the default dependencies later on, to make upgrades easier in general. For the time being, as soon as TF will be unlocked, we'll complete this PR.

@renatomello
Copy link
Contributor

@alecandido
Copy link
Member Author

https://github.com/tensorflow/tensorflow/releases/tag/v2.16.1

On PyPI since Saturday
image

I'll now make the package!

@alecandido
Copy link
Member Author

alecandido commented Mar 11, 2024

Ok, now the situation is even more complicated, since the same problem of QiboJIT is now present for QiboTN as well.

I will move forward with this PR, and the matching ones
qiboteam/qibojit#170
qiboteam/qibotn#48

If tests are passing, we can merge. But we should try to push for qibo-core as well, otherwise this
#1231 (comment)
will get worse and worse.

@alecandido
Copy link
Member Author

alecandido commented Mar 12, 2024

@BrunoLiegiBastonLiegi @renatomello, there is a Clifford test failing at random:
https://github.com/qiboteam/qibo/actions/runs/8241280424/job/22538218856#step:11:433

(as I wrote somewhere else, I expect that is because you're testing with random values, without fixing a seed - so tests are not reproducible; if you want fuzzying, consider using https://hypothesis.readthedocs.io/en/latest/ instead)

@alecandido alecandido marked this pull request as ready for review March 12, 2024 00:10
@alecandido
Copy link
Member Author

And we have a problem with dtypes in Keras, generated by the QGAN tests
https://github.com/qiboteam/qibo/actions/runs/8241324205/job/22538355397?pr=1231#step:11:230

Funnily, the problem is on Linux, but not on Windows.

@renatomello
Copy link
Contributor

@BrunoLiegiBastonLiegi @renatomello, there is a Clifford test failing at random: https://github.com/qiboteam/qibo/actions/runs/8241280424/job/22538218856#step:11:433

(as I wrote somewhere else, I expect that is because you're testing with random values, without fixing a seed - so tests are not reproducible; if you want fuzzying, consider using https://hypothesis.readthedocs.io/en/latest/ instead)

The fix is in #1202

@alecandido
Copy link
Member Author

I guess it is edeff3a. I could cherry-pick it, but I'm not sure how many things I need from #1202.

@renatomello
Copy link
Contributor

I guess it is edeff3a. I could cherry-pick it, but I'm not sure how many things I need from #1202.

#1202 is close to being merged into master. You could just merge master here when it happens, I guess.

In order to avoid the explicit CVXPY dependency
This time to avoid using tf.keras
I.e. remove explicit python version from nix, and remove the mention of CVXPY in qinfo extra, since the dependency is not there any longer
Copy link

codecov bot commented Apr 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.83%. Comparing base (18c155a) to head (2dee381).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1231      +/-   ##
==========================================
- Coverage   99.84%   99.83%   -0.01%     
==========================================
  Files          73       72       -1     
  Lines       10693    10484     -209     
==========================================
- Hits        10676    10467     -209     
  Misses         17       17              
Flag Coverage Δ
unittests 99.83% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alecandido
Copy link
Member Author

alecandido commented Apr 10, 2024

Ok, we just got the first Codecov report for this PR, since it's now passing on Ubuntu (still to be verified on Mac).

In the last commit we have been hit by the cache again*, but it should work in a while.

@stavros11 @scarrazza if you can take a look, and confirm that we're willing to accept the changes, we could merge as soon as the workflows will be passing.

*@scarrazza if you want a report with the cache error, here it is:
https://github.com/qiboteam/qibo/actions/runs/8631159207/attempts/1?pr=1231

Copy link
Member

@stavros11 stavros11 left a comment

Choose a reason for hiding this comment

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

Thanks @alecandido. Other than the comments below, it looks good to me.

Comment on lines +91 to +92
qibojit = { git = "https://github.com/qiboteam/qibojit.git", branch = "py3.12" }
qibotn = { git = "https://github.com/qiboteam/qibotn.git", branch = "py3.12" }
Copy link
Member

Choose a reason for hiding this comment

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

I guess these will be reverted to main later following #1231 (comment). Shall we open an issue before merging this or will we do immediately?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure: unfortunately, that procedure should be followed from every incompatible update, so it could be worth an issue (at least for this time, but even to remember).

However, I'm working in parallel on qibo-core. If I'll manage to have something reasonable soon enough, we might not need anything like this any longer.

@@ -544,22 +546,20 @@ def diamond_norm(channel, target=None, backend=None, **kwargs):
If a ``target`` channel :math:`\\Lambda` is specified,
then it calculates :math:`\\| \\mathcal{E} - \\Lambda\\|_{\\diamond}`.

Example:
Example::
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Example::
Example:

(is the double :: actually needed?)

Copy link
Member Author

Choose a reason for hiding this comment

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

src/qibo/quantum_info/metrics.py Outdated Show resolved Hide resolved
src/qibo/quantum_info/metrics.py Outdated Show resolved Hide resolved
src/qibo/quantum_info/metrics.py Outdated Show resolved Hide resolved
tests/test_quantum_info_metrics.py Show resolved Hide resolved
@alecandido alecandido requested a review from renatomello April 22, 2024 07:41
@alecandido alecandido added this pull request to the merge queue Apr 24, 2024
Merged via the queue into master with commit a90c018 Apr 24, 2024
27 checks passed
@alecandido alecandido deleted the py3.12 branch April 24, 2024 09:11
@alecandido alecandido mentioned this pull request Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants