Skip to content

Conversation

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 28 of 28 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @brosenberg42)


cpp/DarknetDetection/Dockerfile, line 73 at r1 (raw file):

####################################################
FROM darknet_component as sample_executable

What's the motivation for adding this? Just to make manual testing easier before we land the CLI Runner update?


cpp/DarknetDetection/sample_darknet_detector.cpp, line 42 at r1 (raw file):

int main(int argc, char* argv[]) {
    if (argc == 2) {

Should we document this usage in the usage print out?


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

set(USE_AVX_INSTRUCTIONS ON)
set(DLIB_USE_CUDA OFF)

I'm assuming it didn't use CUDA before. Was version 19.20 trying to build with CUDA by default?


cpp/OcvDnnDetection/sample_ocv_dnn_classifier.cpp, line 40 at r1 (raw file):

    if (argc == 2) {
        std::string arg1 = argv[1];
        if (arg1 == "gpu-info") {

Should we document this usage in the usage print out?


cpp/TesseractOCRTextDetection/Dockerfile, line 35 at r1 (raw file):

    && yum --enablerepo=remi install -y ImageMagick ImageMagick-c++ \
         ImageMagick-c++-devel ImageMagick-devel ImageMagick-libs \
         libgif libjpeg libpng libtiff zlib ghostscript \

What motivated you to remove this?

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @brosenberg42)

Copy link
Member Author

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 26 of 34 files reviewed, 5 unresolved discussions (waiting on @brosenberg42 and @jrobble)


cpp/DarknetDetection/Dockerfile, line 73 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

What's the motivation for adding this? Just to make manual testing easier before we land the CLI Runner update?

It was added before CLI runner task was even given to us. I added so I could test on the GPU server.


cpp/DarknetDetection/sample_darknet_detector.cpp, line 42 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Should we document this usage in the usage print out?

Done.


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

Previously, jrobble (Jeff Robble) wrote…

I'm assuming it didn't use CUDA before. Was version 19.20 trying to build with CUDA by default?

I get a build error without that line. I didn't think it was worth my time to figure how to get it running on gpu since no one uses it.


cpp/OcvDnnDetection/sample_ocv_dnn_classifier.cpp, line 40 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Should we document this usage in the usage print out?

Done.


cpp/TesseractOCRTextDetection/Dockerfile, line 35 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

What motivated you to remove this?

It was never used. The build started failing because of yum-config-manager --setopt=skip_missing_names_on_install=False --save;

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


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

Previously, brosenberg42 wrote…

I get a build error without that line. I didn't think it was worth my time to figure how to get it running on gpu since no one uses it.

Agreed.

@brosenberg42 brosenberg42 merged commit cb00ac4 into develop Dec 16, 2020
@brosenberg42 brosenberg42 deleted the feature/opencv4 branch December 16, 2020 15:41
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.

3 participants