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

tflite_micro python package with signal ops #2358

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ericheim
Copy link
Contributor

BUG=#2357

Description

The proposed solution includes signal_ops to the tflite_micro python package. Adding signal_ops introduces a linker dependency to the specific tensorflow-cpu version the tflite_micro package was build against. E.G. when the python package was build against tensorflow-cpu==2.12.0, the version that is used in the requirements.txt, the signal ops will only be able to work when tensorflow=2.12.0 is installed on the system. Tested the build with up to version 2.15.0

Possible improvements

I added the tensorflow target version (2.12.0) to the requirements of the WHEEL and third_party/python_requirements.in. This should probably be done somewhere at a central place. Feedback is welcomed.

Add signal operations to the python package.
@ericheim ericheim requested a review from a team as a code owner December 15, 2023 14:05
@ericheim
Copy link
Contributor Author

Does the CI require a rebase instead of a merge?

@rascani
Copy link
Collaborator

rascani commented Dec 15, 2023

Does the CI require a rebase instead of a merge?

No, merge is fine.

@rascani
Copy link
Collaborator

rascani commented Dec 19, 2023

@ericheim - Just as an FYI, I'm interested in merging this, just working out the ramifications of pinning against a specific version of the tensorflow.

Copy link
Contributor

@rkuester rkuester left a comment

Choose a reason for hiding this comment

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

Hi @ericheim, thanks for this! A few comments:

  • The signal subpackage should be brought into the public API of the tflite_micro package via /python/tflite_micro/__init__.py, like the other subpackages, so it can be addressed as tflite_micro.signal. Without this, it's deeply buried by default at the ugly path tflite_micro.python.tflite_micro.signal.
  • There should be tests which exercise signal from an environment where the package is installed. Please see /python/tflite_micro/whl_test.sh and expand on that idea. The integration tests don't need to revalidate aspects already covered by the unit tests. However, they should cover aspects unique to using signal in an installed environment, as opposed to the bazel environment. (E.g., to make sure modules are found at their installed paths, that addressing as tflite_micro.signal doesn't break things, that nothing was left out of the package, etc.).

@ericheim
Copy link
Contributor Author

Hi @rkuester , @rascani Thanks for your feedback!

@rascani The pinning of the tensorflow version can probably be done somehwere in a central place.
@rkuester import paths in the example tflite-micro/tensorflow/lite/examples/micro_speech can also be changed after adopting the __init__.py.

@rascani
Copy link
Collaborator

rascani commented Dec 20, 2023

The pinning of the tensorflow version can probably be done somehwere in a central place.

Understood, but we have to consider that not all users will be using the same tensorflow version. As I understand it, TF doesn't have any ABI stability guarantees between minor versions (and we're depending on C++ code), so this essentially requires all users to use 2.12. That might be okay, but we need to figure out what the policy should be for this version.

@ericheim
Copy link
Contributor Author

ericheim commented Dec 21, 2023

Hi @rkuester , @rascani ,

I extended the tflite_micro.postinstall_check.passed() with the signal library. Unfortunately adapting the import paths in the __init__.py is a bigger change than expected. This is due to missing __init__.py inside of tflite_micro/python and tflite_micro/python/tflite_micro in the WHEEL file. Using it in dev mode doesn't has these problems.

Adding more code into the existing init files, e.g. directly adding the directories to sys.path interferes either with the tensorflow installation signal ops or breaks the tflite_micro in dev mode, when using it from the folder directly.

@rkuester
Copy link
Contributor

Thanks for adding the test, @ericheim.

I'm working on the import name issue. You're right, adding a subpackage to the public API via the __init__.py trick doesn't work the same way that adding a module works.

@ericheim
Copy link
Contributor Author

ericheim commented Jan 2, 2024

@rkuester Thanks for taking care of the import name issue!

Copy link
Contributor

"This PR is being marked as stale due to inactivity. Remove label or comment to prevent closure in 5 days."

@github-actions github-actions bot added the Stale label Feb 12, 2024
Copy link
Contributor

"This PR is being closed because it has been marked as
stale for 5 days with no further activity."

@github-actions github-actions bot closed this Feb 18, 2024
@rascani rascani removed the Stale label Feb 20, 2024
@rascani rascani reopened this Feb 20, 2024
Copy link
Contributor

github-actions bot commented Apr 1, 2024

"This PR is being marked as stale due to inactivity. Remove label or comment to prevent closure in 5 days."

@github-actions github-actions bot added the Stale label Apr 1, 2024
@rascani rascani removed the Stale label Apr 2, 2024
@ericheim
Copy link
Contributor Author

ericheim commented Apr 3, 2024

Resolved merge conflict. The solution still has to be pinned against a specific tensorflow version.

Copy link
Contributor

"This PR is being marked as stale due to inactivity. Remove label or comment to prevent closure in 5 days."

@github-actions github-actions bot added the Stale label May 14, 2024
@rascani rascani removed the Stale label May 14, 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.

None yet

3 participants