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

python: add 'make generate' OS agnostic #2288

Merged
merged 6 commits into from
Oct 11, 2023
Merged

Conversation

apwojcik
Copy link
Collaborator

@apwojcik apwojcik commented Oct 4, 2023

The Python version of 'make generate' for Linux and Windows.

@apwojcik apwojcik added the Windows Related changes for Windows Environments label Oct 4, 2023
@apwojcik apwojcik added the skip bot checks Skips the Performance and Accuracy CI tests label Oct 4, 2023
@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Merging #2288 (03c1794) into develop (c58e7d8) will decrease coverage by 0.01%.
Report is 2 commits behind head on develop.
The diff coverage is 100.00%.

❗ Current head 03c1794 differs from pull request most recent head a23fbf5. Consider uploading reports for the commit a23fbf5 to get more accurate results

@@             Coverage Diff             @@
##           develop    #2288      +/-   ##
===========================================
- Coverage    91.45%   91.45%   -0.01%     
===========================================
  Files          433      433              
  Lines        16175    16174       -1     
===========================================
- Hits         14793    14792       -1     
  Misses        1382     1382              
Files Coverage Δ
src/api/api.cpp 72.36% <ø> (ø)
src/api/include/migraphx/migraphx.hpp 98.63% <ø> (ø)
src/dynamic_loader.cpp 70.27% <ø> (ø)
src/include/migraphx/auto_register.hpp 100.00% <ø> (ø)
src/include/migraphx/dynamic_loader.hpp 100.00% <ø> (ø)
src/include/migraphx/float_equal.hpp 100.00% <ø> (ø)
src/include/migraphx/generate.hpp 76.92% <ø> (ø)
src/include/migraphx/instruction_ref.hpp 100.00% <ø> (ø)
src/include/migraphx/matcher.hpp 97.13% <ø> (-0.02%) ⬇️
src/include/migraphx/op/nonmaxsuppression.hpp 98.47% <ø> (ø)
... and 5 more

string(MAKE_C_IDENTIFIER ${__rel_name} __cid)
add_custom_target(generate_${__cid} DEPENDS ${__output_native_path})
add_dependencies(generate generate_${__cid})
endfunction()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be written using python instead of cmake? Python is much easier to read. I would like a generate.py script to do the same thing as the generate.sh script.

Copy link
Collaborator Author

@apwojcik apwojcik Oct 5, 2023

Choose a reason for hiding this comment

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

In PR #1893, you asked us to do a Python or CMake script instead of a batch script. We chose CMake because it is more accessible on Windows regarding build configuration and has all the infrastructure for searching for external tools.

@migraphx-bot
Copy link
Collaborator

Test Batch Rate new
3674d8
Rate old
f5411e
Diff Compare
torchvision-resnet50 64 2,325.29 2,323.57 0.07%
torchvision-resnet50_fp16 64 5,355.60 5,356.19 -0.01%
torchvision-densenet121 32 1,844.13 1,832.62 0.63%
torchvision-densenet121_fp16 32 3,419.45 3,419.52 -0.00%
torchvision-inceptionv3 32 1,296.16 1,293.47 0.21%
torchvision-inceptionv3_fp16 32 2,537.89 2,531.69 0.25%
cadene-inceptionv4 16 620.00 620.19 -0.03%
cadene-resnext64x4 16 588.75 588.91 -0.03%
slim-mobilenet 64 7,221.13 7,215.28 0.08%
slim-nasnetalarge 64 236.25 236.38 -0.06%
slim-resnet50v2 64 2,557.08 2,556.82 0.01%
bert-mrpc-onnx 8 824.99 826.08 -0.13%
bert-mrpc-tf 1 387.40 388.61 -0.31%
pytorch-examples-wlang-gru 1 297.69 297.89 -0.07%
pytorch-examples-wlang-lstm 1 311.39 307.72 1.19%
torchvision-resnet50_1 1 548.76 543.08 1.05%
torchvision-inceptionv3_1 1 306.89 305.76 0.37%
cadene-dpn92_1 1 361.31 355.84 1.54%
cadene-resnext101_1 1 219.61 218.19 0.65%
slim-vgg16_1 1 223.87 223.82 0.02%
slim-mobilenet_1 1 1,493.62 1,523.59 -1.97%
slim-inceptionv4_1 1 217.16 215.72 0.67%
onnx-taau-downsample 1 306.42 306.61 -0.06%
dlrm-criteoterabyte 1 21.69 21.69 0.02%
dlrm-criteoterabyte_fp16 1 40.75 40.72 0.07%
agentmodel 1 5,849.73 5,860.43 -0.18%
unet_fp16 2 55.17 55.16 0.01%
resnet50v1_fp16 1 762.96 789.31 -3.34% 🔴
bert_base_cased_fp16 64 971.20 970.39 0.08%
bert_large_uncased_fp16 32 305.01 304.93 0.03%
bert_large_fp16 1 166.83 166.80 0.02%
distilgpt2_fp16 16 1,350.75 1,351.09 -0.03%

This build is not recommended to 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

@apwojcik apwojcik force-pushed the make_generate_script branch from 3674d8f to 68f800a Compare October 6, 2023 11:54
@apwojcik apwojcik changed the title cmake: add 'make generate' Windows compatible python: add 'make generate' OS agnostic Oct 6, 2023
@apwojcik apwojcik requested a review from pfultz2 October 6, 2023 11:56
@apwojcik apwojcik force-pushed the make_generate_script branch 4 times, most recently from 4cb5d9a to 721a0c5 Compare October 6, 2023 12:27
@apwojcik apwojcik force-pushed the make_generate_script branch from 721a0c5 to e63dc69 Compare October 6, 2023 12:39

def api_generate(input_path: Path, output_path: Path):
with open(output_path, 'w') as f:
f.write(clang_format(api.invoke(input_path)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just call the run function directly: f.write(clang_format(api.invoke([migraphx_py_path, input_path]))) then there is no need to call register_functions.

Copy link
Collaborator Author

@apwojcik apwojcik Oct 9, 2023

Choose a reason for hiding this comment

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

I tried that. It fails with an exception after the second call to the original run(), saying "update() not being called". The second call to run() doubles the functions already in the functions list, and @once attached to process_functions() prevents them from being updated. When I removed the decorator/reset the had_run flag, I ended up doubling everything: functions, parameters, structures, etc., in the resulting files. So, either I register functions once or drop everything after each call to api.invoke(). I decided to register functions once.

tools/api.py Outdated
@@ -678,6 +679,10 @@ def add_function(name: str, *args, **kwargs) -> Function:
return f


def register_functions(path: Union[Path, str]) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesnt need to be moved out into a separate function.

tools/te.py Outdated
f = open(sys.argv[1]).read()
r = template_eval(f)
sys.stdout.write(r)
def invoke(p):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Call this run instead of invoke.

@apwojcik apwojcik requested a review from pfultz2 October 9, 2023 09:06
@apwojcik
Copy link
Collaborator Author

apwojcik commented Oct 11, 2023

The cppcheck is failing not because of this PR but because PR #2061 merged before. I fixed the check in #2278

@causten causten merged commit 76bb3b2 into develop Oct 11, 2023
15 checks passed
@causten causten deleted the make_generate_script branch October 11, 2023 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip bot checks Skips the Performance and Accuracy CI tests Windows Related changes for Windows Environments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants