Skip to content

Conversation

cyyever
Copy link
Collaborator

@cyyever cyyever commented Jul 18, 2025

Reland of #157050, which is incidentally closed.

@cyyever cyyever requested review from albanD and soulitzer as code owners July 18, 2025 04:20
Copy link

pytorch-bot bot commented Jul 18, 2025

🔗 Helpful Links

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

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

✅ No Failures

As of commit 782add0 with merge base 1274297 (image):
💚 Looks good so far! There are no failures yet. 💚

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

Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

Looks good from here,
cc @jeanschmidt who reverted the original PR in case you have more information on what was the concern.

@cyyever
Copy link
Collaborator Author

cyyever commented Jul 19, 2025

@albanD I have no permission to access the report so I can only guess some inner code explicitly uses c10::string_view, but should be easy to fix.

@jeanschmidt
Copy link
Contributor

I double-checked, it seems it was raising from executorch tests the error:

    raise RuntimeError(f"Unsupported cpp type {ref.src_cpp_type}")
RuntimeError: Unsupported cpp type ::std::string_view

So I suspect that we might need to coordinate the executorch changes to land internally in sync with those (if there are needed changes from executorch side). I don't have the context to tell if there is need to update executorch or this can be done with retro-compatibility, in the 2nd case I believe we're good to go. Otherwise maybe you will want to codev those changes internally.

Current oncall is @ZainRizvi

@cyyever
Copy link
Collaborator Author

cyyever commented Jul 22, 2025

Thank you,now there is some clue to continue. I can submit related changes there.

@cyyever
Copy link
Collaborator Author

cyyever commented Jul 22, 2025

@jeanschmidt executorch compares with std::string_view, our change here uses ::std::string_view to avoid conflicting with the std operation. Let me check whether std::string_view can work.

@cyyever cyyever force-pushed the svtorchgen2 branch 5 times, most recently from d524569 to 2bf52e5 Compare July 22, 2025 15:45
@albanD albanD added the executorch-needs-help Add this label to your issue/PR if you need help from the ExecuTorch team label Jul 22, 2025
@swolchok
Copy link
Contributor

why not just patch the executorch check so that "::std::string_view" is also acceptable?

elif ref.src_cpp_type == STRING or ref.src_cpp_type == "::" + STRING or ref.src_cpp_type == OLD_STRING:

@cyyever
Copy link
Collaborator Author

cyyever commented Jul 23, 2025

why not just patch the executorch check so that "::std::string_view" is also acceptable?

elif ref.src_cpp_type == STRING or ref.src_cpp_type == "::" + STRING or ref.src_cpp_type == OLD_STRING:

That solution of course works, but if STRING is used in other places we have to repeat the patch. I have proposed a more thorough solution, see
pytorch/executorch#12706.

Signed-off-by: Yuanyuan Chen <[email protected]>
Signed-off-by: Yuanyuan Chen <[email protected]>
Signed-off-by: Yuanyuan Chen <[email protected]>
@cyyever
Copy link
Collaborator Author

cyyever commented Sep 12, 2025

The latest changes don't break executorch and make CI green.

@cyyever
Copy link
Collaborator Author

cyyever commented Sep 12, 2025

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Sep 12, 2025
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@huydhn
Copy link
Contributor

huydhn commented Sep 13, 2025

@pytorchbot revert -m 'Sorry for reverting your change but it seems to break a couple of ExecuTorch tests for Vulkan backend' -c ghfirst

Here is an example failure for executorch/backends/vulkan/test/op_tests:compute_graph_op_tests:

Traceback (most recent call last):
  File "<string>", line 53, in <module>
  File "<string>", line 50, in __run
  File "/re_cwd/buck-out/v2/gen/fbsource/512630cebf016537/xplat/executorch/backends/vulkan/test/op_tests/__generate_op_correctness_tests__/generate_op_correctness_tests#link-tree/__par__/meta_only/bootstrap.py", line 105, in run_as_main
    oss_run_as_main(
  File "/re_cwd/buck-out/v2/gen/fbsource/512630cebf016537/xplat/executorch/backends/vulkan/test/op_tests/__generate_op_correctness_tests__/generate_op_correctness_tests#link-tree/__par__/bootstrap.py", line 70, in run_as_main
    runpy._run_module_as_main(main_module, alter_argv=False)
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/re_cwd/buck-out/v2/gen/fbsource/512630cebf016537/xplat/executorch/backends/vulkan/test/op_tests/__generate_op_correctness_tests__/generate_op_correctness_tests#link-tree/executorch/backends/vulkan/test/op_tests/generate_op_correctness_tests.py", line 90, in <module>
    generate_cpp(args.aten_yaml_path, args.tags_path, args.output)
  File "/usr/local/fbcode/platform010/lib/python3.12/contextlib.py", line 81, in inner
    return func(*args, **kwds)
           ^^^^^^^^^^^^^^^^^^^
  File "/re_cwd/buck-out/v2/gen/fbsource/512630cebf016537/xplat/executorch/backends/vulkan/test/op_tests/__generate_op_correctness_tests__/generate_op_correctness_tests#link-tree/executorch/backends/vulkan/test/op_tests/generate_op_correctness_tests.py", line 75, in generate_cpp
    file.write(cpp_generator.generate_cpp())
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/re_cwd/buck-out/v2/gen/fbsource/512630cebf016537/xplat/executorch/backends/vulkan/test/op_tests/__generate_op_correctness_tests__/generate_op_correctness_tests#link-tree/executorch/backends/vulkan/test/op_tests/utils/gen_correctness_base.py", line 406, in generate_cpp
    test_suites_cpp=self.generate_test_suites_cpp(),
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/re_cwd/buck-out/v2/gen/fbsource/512630cebf016537/xplat/executorch/backends/vulkan/test/op_tests/__generate_op_correctness_tests__/generate_op_correctness_tests#link-tree/executorch/backends/vulkan/test/op_tests/utils/gen_correctness_base.py", line 413, in generate_test_suites_cpp
    return "\n".join([h.generate_suite_cpp() for h in self.suites_gens])
                      ^^^^^^^^^^^^^^^^^^^^^^
  File "/re_cwd/buck-out/v2/gen/fbsource/512630cebf016537/xplat/executorch/backends/vulkan/test/op_tests/__generate_op_correctness_tests__/generate_op_correctness_tests#link-tree/executorch/backends/vulkan/test/op_tests/utils/gen_correctness_base.py", line 264, in generate_suite_cpp
    suite_cpp = self.generate_fixture_cpp()
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/re_cwd/buck-out/v2/gen/fbsource/512630cebf016537/xplat/executorch/backends/vulkan/test/op_tests/__generate_op_correctness_tests__/generate_op_correctness_tests#link-tree/executorch/backends/vulkan/test/op_tests/utils/gen_correctness_vk.py", line 64, in generate_fixture_cpp
    check_fn = self.generator.gen_op_check_fn()
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/re_cwd/buck-out/v2/gen/fbsource/512630cebf016537/xplat/executorch/backends/vulkan/test/op_tests/__generate_op_correctness_tests__/generate_op_correctness_tests#link-tree/executorch/backends/vulkan/test/op_tests/utils/gen_computegraph.py", line 746, in gen_op_check_fn
    op_check_fn_body += self.gen_graph_build_code()
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/re_cwd/buck-out/v2/gen/fbsource/512630cebf016537/xplat/executorch/backends/vulkan/test/op_tests/__generate_op_correctness_tests__/generate_op_correctness_tests#link-tree/executorch/backends/vulkan/test/op_tests/utils/gen_computegraph.py", line 671, in gen_graph_build_code
    graph_build += self.create_value_for(
                   ^^^^^^^^^^^^^^^^^^^^^^
  File "/re_cwd/buck-out/v2/gen/fbsource/512630cebf016537/xplat/executorch/backends/vulkan/test/op_tests/__generate_op_correctness_tests__/generate_op_correctness_tests#link-tree/executorch/backends/vulkan/test/op_tests/utils/gen_computegraph.py", line 484, in create_value_for
    raise RuntimeError(f"Unsupported cpp type {ref.src_cpp_type}")
RuntimeError: Unsupported cpp type ::std::string_view

@huydhn
Copy link
Contributor

huydhn commented Sep 13, 2025

If the tests from ExecuTorch needs to be updated, I could help coordinate their landing

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

@cyyever your PR has been successfully reverted.

pytorchmergebot added a commit that referenced this pull request Sep 13, 2025
This reverts commit 972e409.

Reverted #158625 on behalf of https://github.com/huydhn due to Sorry for reverting your change but it seems to break a couple of ExecuTorch tests for Vulkan backend ([comment](#158625 (comment)))
@cyyever
Copy link
Collaborator Author

cyyever commented Sep 13, 2025

If the tests from ExecuTorch needs to be updated, I could help coordinate their landing

Help me thank you..

markc-614 pushed a commit to markc-614/pytorch that referenced this pull request Sep 17, 2025
Reland of pytorch#157050, which is incidentally closed.
Pull Request resolved: pytorch#158625
Approved by: https://github.com/albanD
markc-614 pushed a commit to markc-614/pytorch that referenced this pull request Sep 17, 2025
This reverts commit 972e409.

Reverted pytorch#158625 on behalf of https://github.com/huydhn due to Sorry for reverting your change but it seems to break a couple of ExecuTorch tests for Vulkan backend ([comment](pytorch#158625 (comment)))
mansiag05 pushed a commit to mansiag05/pytorch that referenced this pull request Sep 22, 2025
Reland of pytorch#157050, which is incidentally closed.
Pull Request resolved: pytorch#158625
Approved by: https://github.com/albanD
mansiag05 pushed a commit to mansiag05/pytorch that referenced this pull request Sep 22, 2025
This reverts commit 972e409.

Reverted pytorch#158625 on behalf of https://github.com/huydhn due to Sorry for reverting your change but it seems to break a couple of ExecuTorch tests for Vulkan backend ([comment](pytorch#158625 (comment)))
cleonard530 pushed a commit to cleonard530/pytorch that referenced this pull request Sep 22, 2025
Reland of pytorch#157050, which is incidentally closed.
Pull Request resolved: pytorch#158625
Approved by: https://github.com/albanD
cleonard530 pushed a commit to cleonard530/pytorch that referenced this pull request Sep 22, 2025
This reverts commit 972e409.

Reverted pytorch#158625 on behalf of https://github.com/huydhn due to Sorry for reverting your change but it seems to break a couple of ExecuTorch tests for Vulkan backend ([comment](pytorch#158625 (comment)))
dsashidh pushed a commit to dsashidh/pytorch that referenced this pull request Sep 26, 2025
Reland of pytorch#157050, which is incidentally closed.
Pull Request resolved: pytorch#158625
Approved by: https://github.com/albanD
dsashidh pushed a commit to dsashidh/pytorch that referenced this pull request Sep 26, 2025
This reverts commit 972e409.

Reverted pytorch#158625 on behalf of https://github.com/huydhn due to Sorry for reverting your change but it seems to break a couple of ExecuTorch tests for Vulkan backend ([comment](pytorch#158625 (comment)))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-no-td Do not run TD on this PR ciflow/trunk Trigger trunk jobs on your pull request executorch-needs-help Add this label to your issue/PR if you need help from the ExecuTorch team Merged open source release notes: inductor (aoti) Reverted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants