Skip to content

Conversation

Sidharth123-cpu
Copy link
Contributor

@Sidharth123-cpu Sidharth123-cpu commented Jun 13, 2025

This PR fixes the gb_id workflow, by integrating with pull.yml and test.sh, in order to be able to build pytorch from source and be able to expand hints, which was the original root cause as to why the previous workflow was breaking.

After much testing with this dummy file:
testing_github_cli.py:

def dummy_method():
unimplemented_v2(
gb_type="Testing gb_type",
context="Testing context",
explanation="testing",
hints=[*graph_break_hints.USER_ERROR],
)

The test check_unimplemented_calls works as intended:

Scenario 1: The callsite is added, however, the entry is not in the json -> test fails and throws the error
Scenario 2: The callsite is added, and the entry is in json -> test passes
Scenario 3: The callsite was never added (meaning the PR never modified any _dynamo files) and the test identifies this and automatically passes since the json file doesn't need to be modified in any way.

Stack from ghstack (oldest at bottom):

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames

@Sidharth123-cpu Sidharth123-cpu requested a review from a team as a code owner June 13, 2025 03:41
Copy link

pytorch-bot bot commented Jun 13, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/155881

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure, 2 Unrelated Failures

As of commit 9c9b4a3 with merge base 0a6b66c (image):

NEW FAILURE - The following job has failed:

UNSTABLE - The following jobs are marked as unstable, possibly due to flakiness on trunk:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames

[ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames

[ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames

[ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames

[ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames

[ghstack-poisoned]
Sidharth123-cpu added a commit that referenced this pull request Jun 13, 2025
ghstack-source-id: 734bd54
Pull Request resolved: #155881
@clee2000
Copy link
Contributor

clee2000 commented Jun 13, 2025

I guess I also don't really understand why it isn't a python unittest

Reply: There isn't really spot to be this as a unittest and I don't think it's really needed to create a new test suite for this, since it's a one-off script in tools/dynamo. A workflow is a better place to put this in since dynamo unit tests are more related to testing its internals and not these type of checks.

This PR fixes the gb_id workflow, by integrating with pull.yml and test.sh, in order to be able to build pytorch from source and be able to expand hints, which was the original root cause as to why the previous workflow was breaking.

After much testing with this dummy file:
testing_github_cli.py:

def dummy_method():
    unimplemented_v2(
        gb_type="Testing gb_type",
        context="Testing context",
        explanation="testing",
        hints=[*graph_break_hints.USER_ERROR],
    )

The test check_unimplemented_calls works as intended:

Scenario 1: The callsite is added, however, the entry is not in the json -> test fails and throws the error
Scenario 2: The callsite is added, and the entry is in json -> test passes
Scenario 3: The callsite was never added (meaning the PR never modified any _dynamo files) and the test identifies this and automatically passes since the json file doesn't need to be modified in any way.





cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames

[ghstack-poisoned]
Sidharth123-cpu added a commit that referenced this pull request Jun 13, 2025
ghstack-source-id: 74b2674
Pull Request resolved: #155881
@Sidharth123-cpu Sidharth123-cpu requested a review from clee2000 June 13, 2025 16:55
@clee2000
Copy link
Contributor

clee2000 commented Jun 13, 2025

I guess I also don't really understand why it isn't a python unittest

Reply: There isn't really spot to be this as a unittest and I don't think it's really needed to create a new test suite for this, since it's a one-off script in tools/dynamo. A workflow is a better place to put this in since dynamo unit tests are more related to testing its internals and not these type of checks.

cc @malfet @albanD re public bindings example?

There are tests like test_public_bindings, which go through the torch package to look for functions (I think) in a list, which seems very similar to this. I also don't really get why creating a new test suite is bad since a new test suite is basically a new file

Reply: The only reason why I'd be opposed to it is because currently all dynamo tests are in test/dynamo and the code I'm testing doesn't reside in torch/_dynamo but rather tools/dynamo. However, if you think it is best to place this as a unittest, it'll be fine to do so.

@Sidharth123-cpu
Copy link
Contributor Author

After some thought and discussion with William, we can move this to be a test suite in tools/dynamo and do something similar to test_dynamo.

@albanD
Copy link
Collaborator

albanD commented Jun 16, 2025

I'm not sure what this test is checking for so hard to give an opinion haha
If that for validation, lint are usually pretty good for these.

@github-actions github-actions bot deleted the gh/Sidharth123-cpu/33/head branch July 19, 2025 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants