[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提交成功，感谢你对开源项目的贡献! |
gouzil
requested review from
SigureMo and
Copilot
and removed request for
Copilot
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).
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:
|
| 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)
…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是否引起精度变化
否