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

Migrate ML backends to Qiboml #1365

Closed
wants to merge 25 commits into from
Closed

Migrate ML backends to Qiboml #1365

wants to merge 25 commits into from

Conversation

MatteoRobbiati
Copy link
Contributor

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

@MatteoRobbiati MatteoRobbiati marked this pull request as ready for review June 19, 2024 09:26
@MatteoRobbiati MatteoRobbiati added this to the Qibo 0.2.9 milestone Jun 19, 2024
Copy link

codecov bot commented Jun 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.82%. Comparing base (ad50a20) to head (750a510).

Current head 750a510 differs from pull request most recent head 72eea4b

Please upload reports for the commit 72eea4b to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1365      +/-   ##
==========================================
- Coverage   99.84%   99.82%   -0.02%     
==========================================
  Files          74       72       -2     
  Lines       10806    10507     -299     
==========================================
- Hits        10789    10489     -300     
- Misses         17       18       +1     
Flag Coverage Δ
unittests 99.82% <100.00%> (-0.02%) ⬇️

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.

@MatteoRobbiati
Copy link
Contributor Author

@BrunoLiegiBastonLiegi @Edoardo-Pedicillo now tests are passing.

Copy link
Contributor

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi left a comment

Choose a reason for hiding this comment

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

Thanks, this is fine, my only concern is whether it is better to specialize the backend loading for qiboml as you are doing here, which allows not to brake all the previous code that was using set_backend("tensorflow"), but creates an asymmetry with the other backend providers, such as qibojit. Or, whether it is preferable to modify the qiboml metabackend to have a platform argument selecting the actual backend out of the ones qiboml provides, this would simplify this PR and maintain the symmetry with the other repos, but will involve setting the backends as set_backend("qiboml", platform="tensorflow"). What do you think @alecandido ?

tests/conftest.py Outdated Show resolved Hide resolved
@alecandido
Copy link
Member

@BrunoLiegiBastonLiegi I'd just go for the uniform scheme, no need to make Qiboml different from the other providers.

We should keep a single instruction, just for "tensorflow", that will check explicitly for it, and, in case, replace the backend with "qiboml", and the platform with "tensorflow".
This should be just a transition device, until we decide to break for good (thus, we'll open an issue about, and schedule for removal).

Moreover, I'd even drop all the QIBO_NON_NATIVE_BACKENDS specification, just rely on the package name (as we're already doing).
If the package is not available, we could catch the error and re-raise, hinting that is not installed, or misspelled (that could even be non-existing).
This will allow future backend providers to avoid at all the need of editing qibo source.

@scarrazza scarrazza modified the milestones: Qibo 0.2.9, Qibo 0.2.10 Jun 25, 2024
@alecandido alecandido mentioned this pull request Jun 26, 2024
4 tasks
@scarrazza scarrazza modified the milestones: Qibo 0.2.10, Qibo 0.2.11 Jul 24, 2024
@MatteoRobbiati MatteoRobbiati marked this pull request as draft August 14, 2024 07:16
@MatteoRobbiati MatteoRobbiati self-assigned this Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants