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

Adding issue template #2541

Merged
merged 6 commits into from
Jan 4, 2024
Merged

Adding issue template #2541

merged 6 commits into from
Jan 4, 2024

Conversation

abhimeda
Copy link
Contributor

@abhimeda abhimeda commented Dec 8, 2023

Hello, this PR is introducing a new issue template for ROCm public repositories to make it easier for users to submit the details for issues and bugs. Problems pertaining to documentation or non-bug related, users have the ability to open a blank issue template to submit their report.

Thank you,
Community Support Team (ML SW SDK)

@abhimeda abhimeda requested a review from umangyadav December 8, 2023 19:00
description: (Optional) If this issue relates to a specific ROCm component, it can be mentioned here.
multiple: true
options:
- Other
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having a list of other teams doesn't make sense, We shouldn't be encouraging issues in one project that are actually a problem in a sibling project. This list seems better at the ROCm/ROCm issue tracker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing this out! What we can do is, set the default component to match the repo name. Would that help?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It shouldn't just be the default, it should be the only option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the component list after speaking to my manager. The component will be implicitly set to AMDMIGraphX because when issues come in from this repo, we have access to the repo name.

@migraphx-bot
Copy link
Collaborator

migraphx-bot commented Dec 8, 2023

Test Batch Rate new
4ec43f
Rate old
179940
Diff Compare
torchvision-resnet50 64 2,834.30 2,835.16 -0.03%
torchvision-resnet50_fp16 64 6,506.69 6,504.61 0.03%
torchvision-densenet121 32 2,104.05 2,091.46 0.60%
torchvision-densenet121_fp16 32 3,662.72 3,666.84 -0.11%
torchvision-inceptionv3 32 1,597.77 1,598.43 -0.04%
torchvision-inceptionv3_fp16 32 2,558.63 2,563.01 -0.17%
cadene-inceptionv4 16 722.33 722.27 0.01%
cadene-resnext64x4 16 692.30 692.48 -0.03%
slim-mobilenet 64 8,334.64 8,326.63 0.10%
slim-nasnetalarge 64 230.62 230.60 0.01%
slim-resnet50v2 64 2,663.70 2,664.41 -0.03%
bert-mrpc-onnx 8 812.97 812.77 0.02%
bert-mrpc-tf 1 387.21 388.67 -0.38%
pytorch-examples-wlang-gru 1 303.90 305.71 -0.59%
pytorch-examples-wlang-lstm 1 313.92 315.55 -0.52%
torchvision-resnet50_1 1 605.91 603.03 0.48%
torchvision-inceptionv3_1 1 344.61 342.81 0.52%
cadene-dpn92_1 1 403.95 404.17 -0.06%
cadene-resnext101_1 1 328.22 328.50 -0.08%
slim-vgg16_1 1 459.17 459.24 -0.01%
slim-mobilenet_1 1 2,120.39 2,125.42 -0.24%
slim-inceptionv4_1 1 213.06 214.03 -0.46%
onnx-taau-downsample 1 306.24 305.67 0.19%
dlrm-criteoterabyte 1 21.59 21.58 0.04%
dlrm-criteoterabyte_fp16 1 40.64 40.64 0.01%
agentmodel 1 6,036.64 5,919.12 1.99%
unet_fp16 2 54.76 54.78 -0.04%
resnet50v1_fp16 1 928.10 923.84 0.46%
bert_base_cased_fp16 64 924.54 924.59 -0.01%
bert_large_uncased_fp16 32 290.63 290.70 -0.02%
bert_large_fp16 1 171.93 171.82 0.06%
distilgpt2_fp16 16 1,281.68 1,282.57 -0.07%

This build is OK for merge ✅

@migraphx-bot
Copy link
Collaborator


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

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

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

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

     ✅ torchvision-resnet50_1: PASSED: MIGraphX meets tolerance

     ✅ torchvision-inceptionv3_1: PASSED: MIGraphX meets tolerance

     ✅ cadene-dpn92_1: PASSED: MIGraphX meets tolerance

     ✅ cadene-resnext101_1: PASSED: MIGraphX meets tolerance

     ✅ slim-vgg16_1: PASSED: MIGraphX meets tolerance

     ✅ slim-mobilenet_1: PASSED: MIGraphX meets tolerance

     ✅ slim-inceptionv4_1: PASSED: MIGraphX meets tolerance

     ✅ dlrm-criteoterabyte: PASSED: MIGraphX meets tolerance

     ✅ agentmodel: PASSED: MIGraphX meets tolerance

     ✅ unet: PASSED: MIGraphX meets tolerance

     ✅ resnet50v1: PASSED: MIGraphX meets tolerance

     ✅ bert_base_cased_fp16: PASSED: MIGraphX meets tolerance

     ✅ bert_large_uncased_fp16: PASSED: MIGraphX meets tolerance

     ✅ bert_large: PASSED: MIGraphX meets tolerance

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

@@ -0,0 +1 @@
blank_issues_enabled: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this mean? Does this mean we can still open issues without the template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes for issues that don't align with the template

- ROCm 5.7.0
- ROCm 5.6.0
- ROCm 5.5.1
- ROCm 5.5.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

We dont really want to maintain a list of rocm versions here. How would a user report the issue with rocm 6.0 or 6.1 that is not on the list? It should just be type: input to write in the rocm version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added ROCm 6.0.0

We've opted for a drop down instead of an input field because it standardizes how it is represented. If we pulled an issue from this repo into a dashboard, we can easily search for ROCm 6.0.0 vs trying to find rocm 6, or ROCm 6, ROCM 6, etc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You cant do a regex like [0-9]+\.[0-9]+\.[0-9]+ to validate the format? I would assume the user would just write 6.0.1(no need to repeat rocm).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This decision was based on how users submitted issues in the RadeonOpenCompute organization. There was too much variation and grouping related issues was difficult.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So how does a user report an issue with 6.1 or 6.2 when they are released? There is no option for those versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would have to submit another PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

But how does the user report it when the PR hasn't been merged yet? Such PRs would be lower priority to merge if we are needing to get features or bug fixes in, so this is a likely scenario. Very likely users will probably pick a random version and then add a comment to which rocm version they are actually using. So this field seems completely useless.

@abhimeda abhimeda requested a review from pfultz2 December 18, 2023 13:43
@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (7091dec) 91.38% compared to head (b574111) 91.39%.
Report is 14 commits behind head on develop.

❗ Current head b574111 differs from pull request most recent head 4ec43fb. Consider uploading reports for the commit 4ec43fb to get more accurate results

Files Patch % Lines
src/include/migraphx/op/nonmaxsuppression.hpp 82.14% 5 Missing ⚠️
src/include/migraphx/op/quant_dot.hpp 50.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #2541   +/-   ##
========================================
  Coverage    91.38%   91.39%           
========================================
  Files          454      456    +2     
  Lines        17189    17250   +61     
========================================
+ Hits         15708    15765   +57     
- Misses        1481     1485    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@abhimeda abhimeda requested a review from causten January 3, 2024 21:01
@causten causten merged commit 496d44b into develop Jan 4, 2024
8 of 9 checks passed
@causten causten deleted the abhimeda-adding-issue-template branch January 4, 2024 16:10
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.

5 participants