refactor: extract checkUUID function for reuse - #1622
Conversation
github-actions
Bot
added
kind/enhancement
Summary of ChangesHello @DSFans2014, 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 streamlines the codebase by centralizing the logic for validating device UUIDs based on pod annotations. By extracting this common functionality into a single utility function, it enhances maintainability and consistency across various device implementations, making the code cleaner and easier to manage. 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. 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
|
Signed-off-by: james <open4pd@4paradigm.com>
bc02b7a to
e2be909
Compare
![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 good refactoring effort that centralizes the checkUUID logic into a common utility function device.CheckUUID, effectively reducing code duplication across various device packages. The implementation of the new function is correct and the refactoring is applied consistently. I've found a minor issue in a test case for the new function that needs to be addressed.
Signed-off-by: james <open4pd@4paradigm.com>
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, DSFans2014 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* refactor: extract checkUUID function for reuse Signed-off-by: james <open4pd@4paradigm.com> * fix: fix test case Signed-off-by: james <open4pd@4paradigm.com> --------- Signed-off-by: james <open4pd@4paradigm.com>
KunlunUseUUID and KunlunNoUseUUID were defined but never read, so a pod asking for baidu.com/use-gpuuuid was scheduled onto any XPU and nouse-gpuuuid could not exclude a card. Every other backend calls device.CheckUUID in Fit(); kunlun was missed when the shared helper landed in Project-HAMi#1622 and again when Project-HAMi#2045 changed it. graghSelect derives topology from a device's position in the slice, so the check goes into the fitFn it already takes rather than filtering the slice, which would shift devices between wings. Both KunlunDevices and KunlunVDevices are covered, and an allocation that fails only because of the annotations now reports CardUuidMismatch instead of NumaNotFit. Signed-off-by: Lakshya77089 <lakshyasharma7708@gmail.com>
* fix(kunlun): honour use-gpuuuid and nouse-gpuuuid annotations KunlunUseUUID and KunlunNoUseUUID were defined but never read, so a pod asking for baidu.com/use-gpuuuid was scheduled onto any XPU and nouse-gpuuuid could not exclude a card. Every other backend calls device.CheckUUID in Fit(); kunlun was missed when the shared helper landed in #1622 and again when #2045 changed it. graghSelect derives topology from a device's position in the slice, so the check goes into the fitFn it already takes rather than filtering the slice, which would shift devices between wings. Both KunlunDevices and KunlunVDevices are covered, and an allocation that fails only because of the annotations now reports CardUuidMismatch instead of NumaNotFit. Signed-off-by: Lakshya77089 <lakshyasharma7708@gmail.com> * fix(kunlun): reuse the xpu uuid annotations and dedupe the mismatch count graghSelect can call fitFn more than once for the same device, so the plain counter reported the same card twice. Track mismatched ids in a map instead. The physical path now reads the same hami.io/use-xpu-uuid and hami.io/no-use-xpu-uuid keys vdevice.go already defines, rather than a new pair, so both kunlun paths honour one documented annotation. baidu.com/ stays as the legacy alias. Added a test that uses the literal key so a rename of the constant cannot silently break it. Signed-off-by: Lakshya77089 <lakshyasharma7708@gmail.com> --------- Signed-off-by: Lakshya77089 <lakshyasharma7708@gmail.com>
3 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Make
checkUUIDfunction as a common utility functionWhich issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: