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

RSDK-9058: remove tflite from rdk #4476

Merged
merged 6 commits into from
Oct 28, 2024

Conversation

bhaney
Copy link
Member

@bhaney bhaney commented Oct 21, 2024

This is a big change for RDK that removes all instances of tflite from the repo. These will be the following breaking changes

  • removing rdk:builtin:tflite_cpu model from the ml_model service. It is now only available as a module
  • removing the ability to build tflite for android through RDK. That will have to be done within the module repo instead.

This also creates a "fake" vision model in order to replace necessary tests in the transform camera.

@randhid FYI

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Oct 21, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Oct 21, 2024
@@ -179,7 +179,7 @@ jobs:
--platform linux/arm/v7 \
-v `pwd`:/rdk \
ghcr.io/viamrobotics/rdk-devenv:armhf-cache \
sudo -Hu testbot bash -lc 'sudo apt-get install -y python3-venv && cd /rdk && go test -v -tags=no_tflite ./...'
sudo -Hu testbot bash -lc 'sudo apt-get install -y python3-venv && cd /rdk && go test -v ./...'
Copy link
Member Author

Choose a reason for hiding this comment

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

@abe-winter I assume it's all right to remove all instances of the no_tflite tag from the RDK now?

Copy link
Member

Choose a reason for hiding this comment

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

yes OR hear me out we can put the no_tflite flag on every file to commemorate this change

Copy link
Member Author

Choose a reason for hiding this comment

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

@abe-winter the vision service register was split into two go files, register.go and register_cgo.go, can they be combined now?

Copy link
Member

Choose a reason for hiding this comment

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

probably -- if you can still make full-static after that change, it's okay to do

@@ -1,5 +1,3 @@
//go:build !no_tflite
Copy link
Member

Choose a reason for hiding this comment

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

cc @hexbabe, now we can probably move the static source to live with its videosourcewrapper counterparts since there's the weird flite test build dependency circus is gone.

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Oct 22, 2024
@@ -5,7 +5,7 @@ FROM $MAIN_TAG:$BASE_TAG as builder
COPY --chown=1000:1000 ./ /tmp/rdk
WORKDIR /tmp/rdk
RUN --mount=type=secret,id=netrc,uid=1000,dst=/home/testbot/.netrc sudo -Hu testbot bash -lc 'if [ `dpkg --print-architecture` = armhf ]; then \
GOFLAGS=-tags=no_tflite make build-go tool-install; \
make build-go tool-install; \
Copy link
Member

Choose a reason for hiding this comment

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

you also want to remove the offending package from /etc/ld_wrapper.sh

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Oct 22, 2024
Copy link
Member

@abe-winter abe-winter left a comment

Choose a reason for hiding this comment

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

was scared off by large diff but remarkably straightforward

@@ -23,7 +23,6 @@ jobs:
brew install x264
brew install jpeg-turbo
brew install ffmpeg
brew install tensorflowlite # Needs to be last
Copy link
Member

Choose a reason for hiding this comment

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

this step is very slow, awesome to remove

Copy link
Member

@kharijarrett kharijarrett left a comment

Choose a reason for hiding this comment

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

LGTM!

@bhaney bhaney merged commit 7167627 into viamrobotics:main Oct 28, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants