[TE/Topology] Support device filtering when dumping topology #1087
Conversation
… for filtering Signed-off-by: popsiclexu <[email protected]> Signed-off-by: zhenxue.xu <[email protected]>
Summary of ChangesHello @popsiclexu, 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 introduces a new feature to the Transfer Engine that allows for more precise control over topology information retrieval. By implementing device filtering, users can now specify particular HCA devices when dumping topology, which streamlines debugging and analysis by focusing on relevant hardware configurations. This enhancement provides greater flexibility and efficiency in managing and understanding system topology. Highlights
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 adds functionality to filter the dumped topology by device name. The changes correctly propagate the device filter from the Python script through the C++ bindings to the transfer engine. My review includes two points: one high-severity issue regarding whitespace handling in the device name filter string which could lead to incorrect filtering, and one medium-severity style issue about a missing newline at the end of a file. Overall, the changes are well-structured and address the feature request.
| std::string TransferEnginePy::getLocalTopology(const char *device_name) { | ||
| pybind11::gil_scoped_release release; | ||
| auto device_name_safe = device_name ? std::string(device_name) : ""; | ||
| auto device_filter = buildDeviceFilter(device_name_safe); |
There was a problem hiding this comment.
The buildDeviceFilter function does not handle whitespace around the device names in the comma-separated list. For an input like "mlx5_0, mlx5_1", this will result in tokens "mlx5_0" and " mlx5_1". The leading space in the second token will cause the device filter to fail for mlx5_1 because the device name matching is exact.
Please update buildDeviceFilter to trim whitespace from each token. Here is a suggested implementation:
std::vector<std::string> buildDeviceFilter(const std::string &device_names) {
std::stringstream ss(device_names);
std::string item;
std::vector<std::string> tokens;
while (getline(ss, item, ',')) {
// trim leading whitespace
size_t first = item.find_first_not_of(" \t\n\r\f\v");
if (std::string::npos == first) {
continue;
}
// trim trailing whitespace
size_t last = item.find_last_not_of(" \t\n\r\f\v");
tokens.push_back(item.substr(first, (last - first + 1)));
}
return tokens;
}| print('Local topology: ', engine.get_local_topology(device_name=args.device_name)) | ||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(main()) |
There was a problem hiding this comment.
This file is missing a final newline character. It's a common convention to end files with a newline, and some tools might not process the last line correctly without it.
| sys.exit(main()) | |
| sys.exit(main()) | |
popsiclexu
force-pushed
the
enhance/te-topo
branch
from
233dbf5 to
81e5b2b
Compare
2 participants
Description
This PR adds support for dumping topology for specific HCA devices only.
Type of Change
How Has This Been Tested?
Dump topology of all devices:
transfer_engine_topology_dumpDump topology of given devices：
transfer_engine_topology_dump --device-name mlx5_0, mlx5_1Checklist