[REFACTOR][TARGET] Cleanup target config#18788
Conversation
Summary of ChangesHello @tqchen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a substantial refactoring of the target configuration handling within the system. The primary goal is to establish a more robust, maintainable, and extensible infrastructure for managing target-specific options. By introducing a dedicated Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring of the target configuration handling. It introduces a new ConfigSchema utility that centralizes validation, defaulting, and canonicalization of target options, replacing a more complex and distributed implementation. The concept of target 'parsers' has been evolved into 'canonicalizers', and many files have been updated to use the new, cleaner API (e.g., Export is renamed to ToConfig, and GetFeature is removed in favor of accessing attributes). This is a substantial improvement for code clarity and maintainability. I have identified a couple of issues in the new implementation that should be addressed.
tqchen
force-pushed
the
target-refactor
branch
from
17c3f5b to
d43c75e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is a major and well-executed refactoring of the target configuration system. The introduction of ConfigSchema greatly simplifies the logic for defining, validating, and canonicalizing target attributes, replacing the previous ad-hoc system. The changes are consistently applied across the codebase, including renaming parsers to canonicalizers and Export to ToConfig, and flattening the features map into attrs. The new API for add_attr_option is much cleaner. The addition of comprehensive tests for ConfigSchema is also a great contribution. I have a couple of suggestions for improving type consistency and code structure.
src/target/target.cc
Outdated
| // Step 3: Extract from_device before schema validation | ||
| int64_t from_device_id = -1; | ||
| if (config.count(kFromDevice)) { | ||
| from_device_id = config[kFromDevice].cast<int64_t>(); | ||
| config.erase(kFromDevice); | ||
| } |
There was a problem hiding this comment.
The from_device attribute is handled before schema validation, which is inconsistent as it's declared as a target attribute in TVM_REGISTER_TARGET_KIND and should ideally be processed by ConfigSchema. This bypasses the validation and error reporting provided by the schema.
To improve consistency and maintainability, I suggest moving the handling of from_device to after schema_.Resolve() is called. This would involve extracting from_device from the attrs map after it's populated from the resolved map.
For example:
// After Step 6 where `attrs` is populated from `resolved`...
// Step 7: If requested, query attributes from the device.
int64_t from_device_id = -1;
if (auto it = attrs.find(kFromDevice); it != attrs.end()) {
from_device_id = it->second.cast<int64_t>();
attrs.erase(it);
}
if (from_device_id >= 0) {
auto device_params = QueryDevice(from_device_id, target.get());
for (const auto& kv : device_params) {
if (attrs.count(kv.first) == 0) {
attrs[kv.first] = kv.second;
}
}
}
target->attrs = attrs;
return target;This would make the handling of from_device consistent with other attributes.
This PR cleans up the target config handling infrastructure. We isolated the schema checking into a common conifg_schema and reorganizes parses into canonicalizer.
tqchen
force-pushed
the
target-refactor
branch
from
d43c75e to
1587c4b
Compare
2 participants
This PR cleans up the target config handling infrastructure. We isolated the schema checking into a common conifg_schema and reorganizes parses into canonicalizer.