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

Add alternative model serializations for inference #100

Merged
merged 47 commits into from
Mar 12, 2024
Merged

Conversation

drubinstein
Copy link
Contributor

@drubinstein drubinstein commented Sep 29, 2023

This PR is a pretty large refactor. One issue we've seen is that not all users are TensorFlow users and TensorFlow is a pretty heavy dependency. To alleviate that, this PR adds in TFLite, CoreML and ONNX serialized versions of the models. PyTorch is not included for now, but we plan to include it in the future.

Instead of TF being a default dependency, depending on the platform, a different model serialization format will be used by defualt. Instead TF is now an extra that can be installed via basic-pitch[tf].

  • On Linux, TFLite will be used via the tflite-runtime package (which only supports linux)
  • On Mac, CoreML will be used via the coremltools package (which only supports mac)
  • On Windows, ONNX will be used via the onnxruntime package (which supports all platforms)

However, if TF is detected on the system, it will be used instead. Ideally, this change will speed up initialization and install times at the cost of a slightly larger binary due to the inclusion of all the new serialized models.

In addition, related to a red herring when failing to install scipy in Python 3.11, I decided to also move from setup.cfg to pyproject.toml format in this PR. I can move that to another PR if desired.

I believe we can call this a minor bump since there are no breaking changes, but we can also call this a major bump due to the possible results changes for users.

@drubinstein drubinstein force-pushed the drubinstein/pt branch 30 times, most recently from d0c91dd to 8601ef9 Compare October 3, 2023 01:14
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@bgenchel bgenchel left a comment

Choose a reason for hiding this comment

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

Most things look good from what I can understand though i'd like to understand a bit more before approving; there is one comment I left for a small change in the init.py text output.

@drubinstein drubinstein requested a review from bgenchel March 12, 2024 13:18
bgenchel
bgenchel previously approved these changes Mar 12, 2024
@drubinstein drubinstein requested a review from bgenchel March 12, 2024 19:41
tox.ini Show resolved Hide resolved
@drubinstein drubinstein merged commit b0d185b into main Mar 12, 2024
20 checks passed
@drubinstein drubinstein deleted the drubinstein/pt branch March 12, 2024 20:31
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.

2 participants