Ubuntu transition #287

brosenberg42 · 2021-11-02T19:41:48Z

Issues:

Related PRs:

This change is

jrobble · 2022-01-28T18:05:48Z

cpp/OcvYoloDetection/Dockerfile, line 66 at r2 (raw file):

# Triton Client build

Should we use the prebuilt client code from this release?

Download link

jrobble

Reviewed 37 of 41 files at r1, 11 of 11 files at r2, 2 of 2 files at r3, 17 of 17 files at r4, 1 of 1 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @brosenberg42)

a discussion (no related file):
When I build a Python component I get an error about building the wheel:

#7 2.487 Building wheels for collected packages: EastTextDetection
#7 2.497   Building wheel for EastTextDetection (setup.py): started
#7 2.735   Building wheel for EastTextDetection (setup.py): finished with status 'error'
#7 2.735   ERROR: Command errored out with exit status 1:
#7 2.735    command: /opt/mpf/plugin-venv/bin/python3 -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/tmp/pip-req-build-chk40lx4/setup.py'"'"'; __file__='"'"'/tmp/pip-req-build-chk40lx4/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' bdist_wheel -d /tmp/pip-wheel-0khv6ct4
#7 2.735        cwd: /tmp/pip-req-build-chk40lx4/
#7 2.735   Complete output (6 lines):
#7 2.735   usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
#7 2.735      or: setup.py --help [cmd1 cmd2 ...]
#7 2.735      or: setup.py --help-commands
#7 2.735      or: setup.py cmd --help
#7 2.735   
#7 2.735   error: invalid command 'bdist_wheel'
#7 2.735   ----------------------------------------
#7 2.735   ERROR: Failed building wheel for EastTextDetection
#7 2.735   Running setup.py clean for EastTextDetection
#7 2.949 Failed to build EastTextDetection
#7 2.952 Installing collected packages: EastTextDetection
#7 2.954     Running setup.py install for EastTextDetection: started
#7 3.500     Running setup.py install for EastTextDetection: finished with status 'done'
#7 3.508 Successfully installed EastTextDetection-6.3
#7 DONE 4.4s

I've seen this for other Python components as well.

Note that the Docker image is still built and usable, but I think we should prevent this error from happening.

I get something similar for the Python component util and API:

#13 24.90 Building wheels for collected packages: mpf-component-api, mpf-component-util
#13 24.90   Building wheel for mpf-component-api (setup.py): started
#13 25.06   Building wheel for mpf-component-api (setup.py): finished with status 'error'
#13 25.06   ERROR: Command errored out with exit status 1:
#13 25.06    command: /opt/mpf/plugin-venv/bin/python3 -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/tmp/pip-req-build-795au7n6/setup.py'"'"'; __file__='"'"'/tmp/pip-req-build-795au7n6/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' bdist_wheel -d /tmp/pip-wheel-oqw4ug2k
#13 25.06        cwd: /tmp/pip-req-build-795au7n6/
#13 25.06   Complete output (6 lines):
#13 25.06   usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
#13 25.06      or: setup.py --help [cmd1 cmd2 ...]
#13 25.06      or: setup.py --help-commands
#13 25.06      or: setup.py cmd --help
#13 25.06   
#13 25.06   error: invalid command 'bdist_wheel'
#13 25.06   ----------------------------------------
#13 25.06   ERROR: Failed building wheel for mpf-component-api
#13 25.06   Running setup.py clean for mpf-component-api
#13 25.25   Building wheel for mpf-component-util (setup.py): started
#13 25.41   Building wheel for mpf-component-util (setup.py): finished with status 'error'
#13 25.41   ERROR: Command errored out with exit status 1:
#13 25.41    command: /opt/mpf/plugin-venv/bin/python3 -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/tmp/pip-req-build-xrcuszy8/setup.py'"'"'; __file__='"'"'/tmp/pip-req-build-xrcuszy8/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' bdist_wheel -d /tmp/pip-wheel-_y7q71oy
#13 25.41        cwd: /tmp/pip-req-build-xrcuszy8/
#13 25.41   Complete output (6 lines):
#13 25.41   usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
#13 25.41      or: setup.py --help [cmd1 cmd2 ...]
#13 25.41      or: setup.py --help-commands
#13 25.41      or: setup.py cmd --help
#13 25.41   
#13 25.41   error: invalid command 'bdist_wheel'
#13 25.41   ----------------------------------------
#13 25.41   ERROR: Failed building wheel for mpf-component-util
#13 25.41   Running setup.py clean for mpf-component-util
#13 25.56 Failed to build mpf-component-api mpf-component-util
#13 25.59 Installing collected packages: mpf-component-api, numpy, opencv-python, pydub, mpf-component-util
#13 25.59     Running setup.py install for mpf-component-api: started
#13 25.83     Running setup.py install for mpf-component-api: finished with status 'done'
#13 29.03     Running setup.py install for mpf-component-util: started
#13 29.36     Running setup.py install for mpf-component-util: finished with status 'done'

This will be addressed in a separate PR for openmpf/openmpf#1485.

cpp/CMakeLists.txt, line 34 at r2 (raw file):

add_subdirectory(OcvFaceDetection)
#add_subdirectory(OalprLicensePlateTextDetection)

Are these commented out due to extra dependencies that need to be installed? If so, create another block of code starting with a line comment explaining that the following components need additional dependencies, and to refer to their Dockerfiles. Then move the commented-out lines under that comment.

cpp/KeywordTagging/CMakeLists.txt, line 32 at r2 (raw file):

include(../ComponentSetup.cmake)

set(PACKAGE_DIR ${CMAKE_CURRENT_BINARY_DIR}/plugin/${PROJECT_NAME})

Lines like these still exist in TrtisDetection and TesseractOCRTextDetection:

set(PACKAGE_DIR ${CMAKE_CURRENT_BINARY_DIR}/plugin/${PROJECT_NAME})

cpp/OcvFaceDetection/OcvFaceDetection.cpp, line 1024 at r2 (raw file):

    }

    if (verbosity > 0) {

The description for VERBOSE states:

VERBOSE = 1: print settings and detection results and VERBOSE = 0: no debugging output.

That's not accurate. Please change to:

VERBOSE = 1: print detection results and VERBOSE = 0: no debugging output.

cpp/OcvYoloDetection/Dockerfile, line 83 at r5 (raw file):

########################################################################
# CLion

Since this is untested with Ubuntu, just remove the clion build target for now. The old build_setup installed the dlib source and Triton client. The new one is just:

FROM ${BUILD_REGISTRY}openmpf_cpp_component_build:${BUILD_TAG} as build_setup

cpp/TesseractOCRTextDetection/Dockerfile, line 73 at r2 (raw file):

RUN --mount=target=.  \
    patch /etc/ImageMagick-6/policy.xml ImageMagick-policy.patch; \

Why is this necessary?

cpp/TesseractOCRTextDetection/ImageMagick-policy.patch, line 6 at r2 (raw file):

in order to avoid to get image with password text

I don't understand this.

cpp/TrtisDetection/Dockerfile, line 33 at r2 (raw file):

ARG MODELS_REGISTRY=openmpf/

FROM ${MODELS_REGISTRY}openmpf_trtis_detection_models:6.3 as models

You can remove this line.

cpp/TrtisDetection/Dockerfile, line 92 at r2 (raw file):

    && sed -i '/add_subdirectory(\.\.\/\.\.\/src\/clients\/python src\/clients\/python)/d' /tmp/tensorrt-inference-server/build/trtis-clients/CMakeLists.txt \
    # Change from: find_package(CURL CONFIG REQUIRED)
    && sed -i '33s/.*/find_package(CURL REQUIRED)/' /tmp/tensorrt-inference-server/src/clients/c++/library/CMakeLists.txt \

Why is this find_package(CURL REQUIRED) change necessary?

cpp/TrtisDetection/run_tests.sh, line 28 at r2 (raw file):

#############################################################################

set -o errexit -o pipefail -o xtrace

Please document how to use this script at the bottom of README.md in a new section and what it does.

Specfically, mention that there are two test executables (TrtisDetectionTest and TrtisDetectionS3Test), and this script only runs one of them. The other requires a MINIO server or some other S3 provider.

brosenberg42

Reviewable status: 50 of 58 files reviewed, 8 unresolved discussions (waiting on @brosenberg42 and @jrobble)

cpp/CMakeLists.txt, line 34 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Are these commented out due to extra dependencies that need to be installed? If so, create another block of code starting with a line comment explaining that the following components need additional dependencies, and to refer to their Dockerfiles. Then move the commented-out lines under that comment.

Done.

cpp/OcvFaceDetection/OcvFaceDetection.cpp, line 1024 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

The description for VERBOSE states:

VERBOSE = 1: print settings and detection results and VERBOSE = 0: no debugging output.

That's not accurate. Please change to:

VERBOSE = 1: print detection results and VERBOSE = 0: no debugging output.

Done.

cpp/OcvYoloDetection/Dockerfile, line 83 at r5 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Since this is untested with Ubuntu, just remove the clion build target for now. The old build_setup installed the dlib source and Triton client. The new one is just:
FROM ${BUILD_REGISTRY}openmpf_cpp_component_build:${BUILD_TAG} as build_setup

Done.

cpp/TesseractOCRTextDetection/Dockerfile, line 73 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Why is this necessary?

By default, ImageMagick is configured to not process PDFs because of a security vulnerability in Ghostscript. That vulnerability is fixed in the version of Ghostscript that is installed in this image.

cpp/TesseractOCRTextDetection/ImageMagick-policy.patch, line 6 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

in order to avoid to get image with password text

I don't understand this.

This is a diff with context. The context is there so that the change can be applied when there are minor modifications to the file or fail when the file is too different. That comment is in the original file. I didn't write it. I also don't understand what it means.

cpp/TrtisDetection/Dockerfile, line 33 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

You can remove this line.

Done.

cpp/TrtisDetection/Dockerfile, line 92 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Why is this find_package(CURL REQUIRED) change necessary?

It is needed because of how the curl package gets installed. CMake won't find it without that change.

cpp/TrtisDetection/run_tests.sh, line 28 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Please document how to use this script at the bottom of README.md in a new section and what it does.

Specfically, mention that there are two test executables (TrtisDetectionTest and TrtisDetectionS3Test), and this script only runs one of them. The other requires a MINIO server or some other S3 provider.

Done.

jrobble

Reviewed 8 of 8 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @brosenberg42)

brosenberg42 added 2 commits October 28, 2021 11:45

Ubuntu transition

a9367cc

Merge branch 'develop' into feature/ubuntu

3802d0b

brosenberg42 self-assigned this Nov 2, 2021

This was referenced Nov 2, 2021

Ubuntu transition openmpf/openmpf#1447

Merged

Ubuntu transition openmpf/openmpf-docker#157

Merged

Ubuntu transition openmpf/openmpf-contrib-components#76

Merged

Ubuntu transition openmpf/openmpf-cpp-component-sdk#108

Merged

brosenberg42 requested a review from jrobble November 2, 2021 19:46

brosenberg42 added 2 commits January 6, 2022 11:18

Update tika version

709405b

Update Jackson version

d1e1ab4

brosenberg42 mentioned this pull request Jan 18, 2022

Ubuntu Transition openmpf/openmpf-java-component-sdk#56

Merged

brosenberg42 added 2 commits January 18, 2022 13:18

Merge branch 'develop' into feature/ubuntu

62ef27c

Merge branch 'develop' into feature/ubuntu

2ebafa2

brosenberg42 added 3 commits January 31, 2022 11:47

Use pre-builit Triton client.

0ff7dec

Merge branch 'develop' into feature/ubuntu

5f280bd

Upgrade to C++ 17

d1fe7c9

jrobble mentioned this pull request Feb 18, 2022

Add libraries to enable OCV DNN CUDA support. #291

Merged

jrobble and others added 3 commits February 18, 2022 13:53

Add libraries to enable OCV DNN CUDA support. (#291)

3f1e968

Update versions

53ab26c

Update versions

a291e13

jrobble requested changes Mar 29, 2022

View reviewed changes

Address PR issues.

6c667a3

brosenberg42 commented Mar 30, 2022

View reviewed changes

jrobble approved these changes Mar 30, 2022

View reviewed changes

brosenberg42 merged commit a75b860 into develop Mar 30, 2022

brosenberg42 deleted the feature/ubuntu branch March 30, 2022 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ubuntu transition #287

Ubuntu transition #287

brosenberg42 commented Nov 2, 2021 •

edited

jrobble commented Jan 28, 2022

jrobble left a comment

brosenberg42 left a comment

jrobble left a comment

Ubuntu transition #287

Ubuntu transition #287

Conversation

brosenberg42 commented Nov 2, 2021 • edited

jrobble commented Jan 28, 2022

jrobble left a comment

Choose a reason for hiding this comment

brosenberg42 left a comment

Choose a reason for hiding this comment

jrobble left a comment

Choose a reason for hiding this comment

brosenberg42 commented Nov 2, 2021 •

edited