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

remove Tensorflow dependency #750

Merged
merged 4 commits into from
Mar 14, 2024
Merged

remove Tensorflow dependency #750

merged 4 commits into from
Mar 14, 2024

Conversation

Edoardo-Pedicillo
Copy link
Contributor

This PR closes #734

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.
  • Compatibility with Qibo modules (Please edit this section if the current pull request is not compatible with the following branches).
    • Qibo: master
    • Qibolab: main
    • Qibolab_platforms_qrc: main

Copy link

codecov bot commented Mar 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.73%. Comparing base (b811472) to head (00afd66).
Report is 7 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #750   +/-   ##
=======================================
  Coverage   96.73%   96.73%           
=======================================
  Files         108      108           
  Lines        7498     7498           
=======================================
  Hits         7253     7253           
  Misses        245      245           
Flag Coverage Δ
unittests 96.73% <ø> (ø)

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

Files Coverage Δ
src/qibocal/fitting/classifier/run.py 97.47% <ø> (ø)
...bocal/protocols/characterization/classification.py 100.00% <ø> (ø)

@Edoardo-Pedicillo Edoardo-Pedicillo marked this pull request as ready for review March 12, 2024 17:04
pyproject.toml Outdated
matplotlib = { version = "^3.7.0", optional = true }
seaborn = { version = "^0.12.2", optional = true }
pydot = { version = "^1.4.2", optional = true }
tensorflow = { version = "^2.12.0", optional = true, markers = "sys_platform == 'linux' or sys_platform == 'darwin'" }
# TODO: the marker is a temporary solution due to the lack of the tensorflow-io 0.32.0's wheels for Windows, this package is one of
# the tensorflow requirements
skl2onnx = { version = "^1.14.0", optional = true }
onnxruntime = { version = "^1.14.1", optional = true }
onnx = { version = "^1.13.1", optional = true }
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need onnx? Is there a different way of serializing Scikit-Learn objects? (maybe even just dictionaries for the hyperparams, and dump in json)

Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

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

Thanks @Edoardo-Pedicillo, I only had that comment, and it could be split to a separate issue/PR as well. The PR itself seems alright.

@alecandido
Copy link
Member

alecandido commented Mar 13, 2024

Consider that the dump() function, for which you need ONNX at the moment (even in SciKit you need the runtime), seems not to be used.

dump = scikit_utils.scikit_dump

At least not in run.py.

But please, also check yourself.

@Edoardo-Pedicillo
Copy link
Contributor Author

The dump() function is used in the classification routine

classifier.dump()(self.models[qubit][i], dump_dir)

@alecandido
Copy link
Member

The dump() function is used in the classification routine

Correct, that's why I was not finding it in classifiers. So, you need of course to dump the result of the fit. But is there no intrinsic way of Scikit-Learn to serialize this? (I mean, w/o ONNX)

@Edoardo-Pedicillo
Copy link
Contributor Author

Pikle ;) btw onnx is suggested by scikit itself (https://scikit-learn.org/stable/model_persistence.html) and even the other options require extra dependencies

@andrea-pasquale andrea-pasquale added the dependencies Pull requests that update a dependency file label Mar 13, 2024
@andrea-pasquale andrea-pasquale added this to the Qibocal 0.0.8 milestone Mar 13, 2024
@alecandido
Copy link
Member

I was wondering if there was something like a .params attribute to dump and load from JSON. But I would avoid going too much into the detail ourselves. Auditing dependencies is a good practice, but also software reuse (i.e. not reinventing the wheel).

There is also PMML (XML instead of binary) and skops, but they do not seem to be more maintained than scikit-onnx. And onnxruntime is incredibly more maintained and reliable than any of the previous ones.

Let's keep it as it is.

@Edoardo-Pedicillo Edoardo-Pedicillo added this pull request to the merge queue Mar 14, 2024
Merged via the queue into main with commit 587b631 Mar 14, 2024
21 checks passed
@Edoardo-Pedicillo Edoardo-Pedicillo deleted the remove_nn branch March 14, 2024 13:19
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove TensorFlow dependency
3 participants