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

Example PR on how we add SQLites 3 support in Windows #2329

Closed
wants to merge 2 commits into from

Conversation

apwojcik
Copy link
Collaborator

That PR exemplifies how we have added SQLite3 dependency in CMake.

@apwojcik apwojcik added the Windows Related changes for Windows Environments label Oct 13, 2023
@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

Merging #2329 (1c977f8) into develop (f8bf7bd) will not change coverage.
The diff coverage is n/a.

❗ Current head 1c977f8 differs from pull request most recent head 514bd67. Consider uploading reports for the commit 514bd67 to get more accurate results

@@           Coverage Diff            @@
##           develop    #2329   +/-   ##
========================================
  Coverage    91.29%   91.29%           
========================================
  Files          436      436           
  Lines        16335    16335           
========================================
  Hits         14913    14913           
  Misses        1422     1422           

@migraphx-bot
Copy link
Collaborator

migraphx-bot commented Oct 13, 2023

Test Batch Rate new
514bd6
Rate old
e27efe
Diff Compare
torchvision-resnet50 64 2,324.44 2,324.21 0.01%
torchvision-resnet50_fp16 64 5,343.17 5,360.67 -0.33%
torchvision-densenet121 32 1,845.07 1,839.62 0.30%
torchvision-densenet121_fp16 32 3,413.62 3,399.12 0.43%
torchvision-inceptionv3 32 1,292.39 1,292.92 -0.04%
torchvision-inceptionv3_fp16 32 2,532.53 2,541.50 -0.35%
cadene-inceptionv4 16 620.25 620.79 -0.09%
cadene-resnext64x4 16 588.79 590.07 -0.22%
slim-mobilenet 64 7,212.81 7,208.36 0.06%
slim-nasnetalarge 64 236.48 236.62 -0.06%
slim-resnet50v2 64 2,556.79 2,558.40 -0.06%
bert-mrpc-onnx 8 825.55 825.21 0.04%
bert-mrpc-tf 1 389.18 388.05 0.29%
pytorch-examples-wlang-gru 1 296.43 295.10 0.45%
pytorch-examples-wlang-lstm 1 315.12 315.61 -0.16%
torchvision-resnet50_1 1 545.93 550.88 -0.90%
torchvision-inceptionv3_1 1 303.43 301.09 0.78%
cadene-dpn92_1 1 355.03 351.13 1.11%
cadene-resnext101_1 1 219.61 220.86 -0.57%
slim-vgg16_1 1 223.91 224.10 -0.08%
slim-mobilenet_1 1 1,499.53 1,482.35 1.16%
slim-inceptionv4_1 1 218.88 217.99 0.41%
onnx-taau-downsample 1 307.15 306.69 0.15%
dlrm-criteoterabyte 1 21.71 21.72 -0.07%
dlrm-criteoterabyte_fp16 1 40.65 40.76 -0.28%
agentmodel 1 5,759.17 5,786.20 -0.47%
unet_fp16 2 55.80 55.81 -0.01%
resnet50v1_fp16 1 773.56 756.60 2.24%
bert_base_cased_fp16 64 971.31 971.33 -0.00%
bert_large_uncased_fp16 32 305.24 305.14 0.03%
bert_large_fp16 1 166.84 166.81 0.02%
distilgpt2_fp16 16 1,278.62 1,280.57 -0.15%

This build is OK for merge ✅

@migraphx-bot
Copy link
Collaborator


    :white_check_mark:bert-mrpc-onnx: PASSED: MIGraphX meets tolerance

    :white_check_mark:bert-mrpc-tf: PASSED: MIGraphX meets tolerance

    :white_check_mark:pytorch-examples-wlang-gru: PASSED: MIGraphX meets tolerance

    :white_check_mark:pytorch-examples-wlang-lstm: PASSED: MIGraphX meets tolerance

    :white_check_mark:torchvision-resnet50_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:torchvision-inceptionv3_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:cadene-dpn92_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:cadene-resnext101_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:slim-vgg16_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:slim-mobilenet_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:slim-inceptionv4_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:dlrm-criteoterabyte: PASSED: MIGraphX meets tolerance

    :white_check_mark:agentmodel: PASSED: MIGraphX meets tolerance

    :white_check_mark:unet: PASSED: MIGraphX meets tolerance

    :white_check_mark:resnet50v1: PASSED: MIGraphX meets tolerance

🔴bert_base_cased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output


🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output


    :white_check_mark:bert_large: PASSED: MIGraphX meets tolerance

🔴distilgpt2_fp16: FAILED: MIGraphX is not within tolerance - check verbose output

BUILD_COMMAND ${NMAKE_EXECUTABLE} /f ..\\sqlite3\\Makefile.msc USE_AMALGAMATION=1 NO_TCL=1 TOP=..\\sqlite3 libsqlite3.lib
INSTALL_COMMAND "")

ExternalProject_Get_Property(sqlite3 BINARY_DIR)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Building the dependency should be done in a superproject, not directly in our build scripts.

Copy link
Collaborator Author

@apwojcik apwojcik Oct 16, 2023

Choose a reason for hiding this comment

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

SQLite3 authors do not provide CMake, only configure scripts for Linux/GNU and NMake files for Windows. Using ExternalProject is an approved technique for non-CMake compatible dependency projects.

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

if(NOT WIN32)
find_package(PkgConfig REQUIRED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can probably switch to find_package(SQLite3), since we use a cmake build for sqlite.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, we cannot. To use find_package(), the CMake has to generate config mode files. The CMake from rocm-recipes does not do that.

@apwojcik apwojcik closed this Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Windows Related changes for Windows Environments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants