[Compat] Update DispatchKey usage to c10::DispatchKey and replace dispatch_key_to_string with c10::toString#78525
Conversation
…patch_key_to_string with c10::toString
|
你的PR提交成功,感谢你对开源项目的贡献! |
There was a problem hiding this comment.
Pull request overview
This PR aims to align the compat Torch library registration API with the upstream-style dispatch key type by switching from a local DispatchKey enum to c10::DispatchKey, and replacing the local dispatch_key_to_string() helper with c10::toString() for logging/printing.
Changes:
- Add
c10/core/DispatchKey.hinclude and switch operator implementation dispatch keys toc10::DispatchKey. - Replace
dispatch_key_to_string(...)usages withc10::toString(...). - Remove the local
DispatchKeyenum and string conversion helper.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| paddle/phi/api/include/compat/torch/library.h | Switch operator registry dispatch-key storage to c10::DispatchKey and remove local DispatchKey enum/helper. |
| paddle/phi/api/include/compat/torch/library.cpp | Update logging/printing from dispatch_key_to_string to c10::toString. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -696,7 +680,7 @@ class PADDLE_API OperatorRegistry { | |||
| const std::string& schema); | |||
|
|
|||
| void register_implementation(const std::string& qualified_name, | |||
| DispatchKey key, | |||
| c10::DispatchKey key, | |||
| CppFunction&& func); | |||
There was a problem hiding this comment.
DispatchKey enum and dispatch_key_to_string() were removed, but this header still uses DispatchKey in multiple places (e.g., Library constructor/member, value_or(DispatchKey::CPU), and the TORCH_LIBRARY_IMPL macro uses torch::DispatchKey::k). As-is, this will not compile. Please either switch all remaining uses to c10::DispatchKey (and update DispatchKey::CPU etc), or introduce using DispatchKey = c10::DispatchKey; in namespace torch to keep the existing torch::DispatchKey references working (and update the .cpp definitions and tests accordingly).
| #include <type_traits> | ||
| #include <unordered_map> | ||
| #include <vector> | ||
| #include "c10/core/DispatchKey.h" |
There was a problem hiding this comment.
Include style is inconsistent here: other external headers in this file use angle brackets (e.g., <c10/macros/Macros.h>), but DispatchKey.h is included with quotes. Prefer #include <c10/core/DispatchKey.h> to match the rest of the compat headers and avoid accidental relative-include resolution.
| #include "c10/core/DispatchKey.h" | |
| #include <c10/core/DispatchKey.h> |
| void OperatorRegistry::register_implementation( | ||
| const std::string& qualified_name, DispatchKey key, CppFunction&& func) { | ||
| auto& op = get_or_create_operator(qualified_name); | ||
| op.implementations[key] = std::move(func); | ||
| VLOG(3) << "Registered implementation: " << qualified_name << " for " | ||
| << dispatch_key_to_string(key); | ||
| << c10::toString(key); | ||
| } |
There was a problem hiding this comment.
OperatorRegistry::register_implementation definition still takes DispatchKey key, but the header now declares c10::DispatchKey key. This is an ODR/compile error. Update the .cpp signature (and any remaining DispatchKey uses in this translation unit) to match the header/type decision.
| // Library | ||
| Library::Library(Kind kind, | ||
| const std::string& ns, | ||
| std::optional<DispatchKey> dispatch_key, | ||
| const char* file, | ||
| uint32_t line) | ||
| : kind_(kind), | ||
| ns_(ns), | ||
| dispatch_key_(dispatch_key), | ||
| file_(file), | ||
| line_(line) { | ||
| std::stringstream oss; | ||
| oss << "Created Library: kind=" << kind_to_string(kind) | ||
| << ", namespace=" << ns; | ||
| if (dispatch_key) { | ||
| oss << ", dispatch_key=" << dispatch_key_to_string(*dispatch_key); | ||
| oss << ", dispatch_key=" << c10::toString(*dispatch_key); | ||
| } |
There was a problem hiding this comment.
Library::Library(...) definition still uses std::optional<DispatchKey> for dispatch_key, but the PR is moving toward c10::DispatchKey and the header currently still references DispatchKey in several places. Once DispatchKey is removed/aliased, this will fail to compile. Please align the constructor parameter/member types with the chosen dispatch key type (e.g., std::optional<c10::DispatchKey> or torch::DispatchKey alias).
There was a problem hiding this comment.
感谢这次清理。我检查了 diff、PR 当前 head,以及当前 CI 状态。
这个 PR 的目标很明确:把这条 compat torch 注册路径从本地的 DispatchKey 枚举 / dispatch_key_to_string() 辅助函数,迁移到 c10::DispatchKey / c10::toString()。
但就当前 head 而言,这次迁移还没有完成。paddle/phi/api/include/compat/torch/library.h 和 paddle/phi/api/include/compat/torch/library.cpp 在已修改 hunk 之外,仍然保留了多处 DispatchKey / torch::DispatchKey 的引用,因此这个补丁目前还没有做到内部一致,较大概率会破坏这条 compat 路径的编译。我已经在行内评论里列出了具体还残留的旧用法位置。
另外,在我审查时,required approval check 仍在等待指定 approver,PR template check 失败,主构建/测试任务也还在运行中。
| } | ||
| } | ||
|
|
||
| // Operator Registration |
There was a problem hiding this comment.
这里移除本地 DispatchKey 之后,仍有多处未修改的调用点继续引用旧类型,因此当前这组改动还没有做到内部一致。按 PR 当前 head,Library / TorchLibraryInit 仍然使用 std::optional<DispatchKey>,def() / impl() 仍然在做 value_or(DispatchKey::CPU),TORCH_LIBRARY_IMPL 仍然会展开成 torch::DispatchKey::k,而且 library.cpp 里也依然保留了 register_implementation(..., DispatchKey) 和 Library(..., std::optional<DispatchKey>) 这样的旧签名。
除非你在 namespace torch 里补一个别名,或者在同一个 PR 里把这些剩余引用也一并改成 c10::DispatchKey,否则这条 compat 注册路径会编译不过。
There was a problem hiding this comment.
我对比了一下 torch,发现他们都是使用 c10::DispatchKey, 我们在这里保留这个 namespace 不太合适
There was a problem hiding this comment.
我对比了一下 torch,发现他们都是使用
c10::DispatchKey, 我们在这里保留这个 namespace 不太合适
同意要对齐到 c10::DispatchKey。但我复看了最新 head,当前只把 TORCH_LIBRARY_IMPL 宏和测试里的查找键改成了 c10::DispatchKey,torch::Library / detail::TorchLibraryInit / library.cpp 的签名和成员仍然是裸 DispatchKey。
具体还残留在 library.h:734/756/767/786/822 和 library.cpp:237/271。另外 compat c10/core/DispatchKey.h 当前并没有把 c10::DispatchKey re-export 到 namespace torch,这里只有 using c10::kAutograd,所以现在仍然和 upstream 不一致,也仍有编译风险。
There was a problem hiding this comment.
我对比了一下 torch,发现他们都是使用
c10::DispatchKey, 我们在这里保留这个 namespace 不太合适
已复核最新 head。现在 library.h/.cpp、detail::TorchLibraryInit、Library 的构造参数与成员、TORCH_LIBRARY_IMPL 宏,以及测试里的 dispatch key 查找都已经统一到 c10::DispatchKey,同时 include 也改成了 <c10/core/DispatchKey.h>。这次实现已经和 PyTorch upstream 的对应路径对齐,我这边会把之前这条阻塞意见收口。
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #78525 +/- ##
==========================================
Coverage ? 83.33%
==========================================
Files ? 2
Lines ? 6
Branches ? 0
==========================================
Hits ? 5
Misses ? 1
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ShigureNyako
left a comment
There was a problem hiding this comment.
我按最新 head 和作者回复又复核了一轮,重点对照了 PyTorch upstream 的 torch/library.h。方向是对的:upstream 确实统一使用 c10::DispatchKey,_TORCH_LIBRARY_IMPL 也应传 c10::DispatchKey::k。本次新增提交已经把宏和测试里的查找键改到了这一路径,这部分与 upstream 一致。
但 migration 仍未完整收口,所以我这轮仍然不能通过。当前 PR head 中:
paddle/phi/api/include/compat/torch/library.h:734, 756, 767, 786, 822还在使用裸DispatchKey/DispatchKey::CPUpaddle/phi/api/include/compat/torch/library.cpp:237, 271仍然使用裸DispatchKey- 同时 compat
paddle/phi/api/include/compat/c10/core/DispatchKey.h:586-588并没有把DispatchKeyre-export 到namespace torch
而 upstream pytorch/torch/library.h 对应位置仍是 std::optional<c10::DispatchKey>(例如构造函数、member、detail::TorchLibraryInit),并在 _TORCH_LIBRARY_IMPL 中传 std::make_optional(c10::DispatchKey::k)。
因此当前实现仍然存在 API/类型不一致:宏和测试已经切到 c10::DispatchKey,但 torch::Library 这条 compat 注册路径内部还保留旧的裸类型名,兼容性与编译语义都没有真正与 PyTorch 对齐。请把这些剩余位置也统一改成 c10::DispatchKey(或者显式引入别名,但这会偏离 upstream 设计)。
CI 方面,最新 head 的 PR template / pre-commit 已通过;required approval check 仍未满足,主要 build/test 任务也还在运行。
…om a relative path to an absolute path
ShigureNyako
left a comment
There was a problem hiding this comment.
我复核了最新 head。之前指出的几处关键问题已经收口:
library.h/.cpp中Library、detail::TorchLibraryInit、register_implementation()、成员与默认值都已统一为c10::DispatchKeyTORCH_LIBRARY_IMPL已改为传std::make_optional(c10::DispatchKey::k)- include 已统一为
<c10/core/DispatchKey.h> - 测试也补到了运行时注册和
Library带 dispatch key 的路径
我对照了 PyTorch upstream torch/library.h,当前这条 compat torch 注册路径在 DispatchKey 的类型与宏语义上已经对齐。CI 里除 approval 外,其余 required 流水线当前均为通过或运行中;就代码实现本身看,我这边通过。
| op.implementations[key] = std::move(func); | ||
| VLOG(3) << "Registered implementation: " << qualified_name << " for " | ||
| << dispatch_key_to_string(key); | ||
| << c10::toString(key); |
…ispatch_key_to_string with `c10::toString` (PaddlePaddle#78525)

PR Category
Execute Infrastructure
PR Types
Devs
Description
移除
paddle/phi/api/include/compat/torch/library.h下的DispatchKey, 改用c10::DispatchKey是否引起精度变化
否