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

add 'make generate' script for Windows #1893

Closed
wants to merge 1 commit into from

Conversation

apwojcik
Copy link
Collaborator

The PR from the series to enable MIGraphX on Windows.

@apwojcik apwojcik requested review from pfultz2 and umangyadav June 27, 2023 21:02
@apwojcik apwojcik force-pushed the generate_batch_script branch from 14d38dc to fca43d9 Compare June 27, 2023 21:38
@pfultz2
Copy link
Collaborator

pfultz2 commented Jun 27, 2023

This should be either a python or cmake script so it uses the same script for both windows and linux.

@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #1893 (ae2a183) into develop (697709a) will not change coverage.
The diff coverage is n/a.

❗ Current head ae2a183 differs from pull request most recent head 82f6680. Consider uploading reports for the commit 82f6680 to get more accurate results

@@           Coverage Diff            @@
##           develop    #1893   +/-   ##
========================================
  Coverage    91.37%   91.37%           
========================================
  Files          419      419           
  Lines        15544    15544           
========================================
  Hits         14204    14204           
  Misses        1340     1340           

@migraphx-bot
Copy link
Collaborator

migraphx-bot commented Jun 28, 2023

Test Batch Rate new
82f668
Rate old
697709
Diff Compare
torchvision-resnet50 64 895.43 895.75 -0.04%
torchvision-resnet50_fp16 64 5,309.84 5,314.47 -0.09%
torchvision-densenet121 32 1,126.36 1,126.59 -0.02%
torchvision-densenet121_fp16 32 3,284.62 3,284.10 0.02%
torchvision-inceptionv3 32 592.83 592.96 -0.02%
torchvision-inceptionv3_fp16 32 2,512.33 2,517.46 -0.20%
cadene-inceptionv4 16 329.05 329.93 -0.27%
cadene-resnext64x4 16 394.60 393.50 0.28%
slim-mobilenet 64 7,128.93 7,139.23 -0.14%
slim-nasnetalarge 64 159.83 159.91 -0.05%
slim-resnet50v2 64 1,089.34 1,089.62 -0.03%
bert-mrpc-onnx 8 719.12 719.61 -0.07%
bert-mrpc-tf 1 370.19 372.21 -0.54%
pytorch-examples-wlang-gru 1 306.91 307.29 -0.12%
pytorch-examples-wlang-lstm 1 306.90 313.39 -2.07%
torchvision-resnet50_1 1 91.70 91.58 0.13%
torchvision-inceptionv3_1 1 130.41 129.13 0.99%
cadene-dpn92_1 1 334.39 335.84 -0.43%
cadene-resnext101_1 1 236.97 236.90 0.03%
slim-vgg16_1 1 53.39 53.37 0.03%
slim-mobilenet_1 1 1,476.36 1,526.43 -3.28% 🔴
slim-inceptionv4_1 1 101.94 99.82 2.12%
onnx-taau-downsample 1 316.20 316.86 -0.21%
dlrm-criteoterabyte 1 21.44 21.44 0.03%
dlrm-criteoterabyte_fp16 1 39.97 39.97 -0.00%
agentmodel 1 5,808.42 5,625.18 3.26% 🔆
unet_fp16 2 52.72 52.81 -0.17%

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

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


    :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

::
:: The MIT License (MIT)
::
:: Copyright (c) 2015-2022 Advanced Micro Devices, Inc. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

2023

@apwojcik apwojcik added the Windows Related changes for Windows Environments label Sep 19, 2023
@tperry-amd
Copy link
Contributor

This should be either a python or cmake script so it uses the same script for both windows and linux.

I agree, a unified script for Linux and Windows is preferred. Our mandate for these initial changes was to minimally impact Linux. Are you okay if we generate a work item to address this after all the Windows changes have made it upstream? The reasoning is that this change is blocking Windows and the work for a new unified script has a testing requirement for both Windows and Linux that will take some time and is lower priority compared to enabling the larger Windows effort.

@pfultz2
Copy link
Collaborator

pfultz2 commented Sep 26, 2023

The reasoning is that this change is blocking Windows

make generate isnt needed to build migraphx either on windows or linux. Its only needed when a developer changes one of the autogenerated files, which can be done on linux at PR time. So I dont see how this could be a blocker, especially if all the changes has been upstreamed already.

@tperry-amd
Copy link
Contributor

tperry-amd commented Sep 26, 2023

The reasoning is that this change is blocking Windows

make generate isnt needed to build migraphx either on windows or linux. Its only needed when a developer changes one of the autogenerated files, which can be done on linux at PR time. So I dont see how this could be a blocker, especially if all the changes has been upstreamed already.

Fair point. It is still additional work that affects Linux more directly. Our goal for the initial changes was to minimally impact Linux and follow your patterns. The pattern here was to solve this problem with a shell script and we solved it with a shell script for Windows. We can change the pattern and create a unified script that works on both OS but it could potentially affect Linux which increases the testing work.

I think we should consider a change in goal. A modest increase in Linux risk is okay if we create a unified solution. We'll pull the change, move it to another branch, and hold onto the branch until we have time to do the unified solution.

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.

5 participants