-
-
Notifications
You must be signed in to change notification settings - Fork 11.6k
Add @tjtanaa to codeowner for ROCm and multi-modal #28360
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: tjtanaa <tunjian.tan@embeddedllm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds @tjtanaa as a codeowner for ROCm and multi-modal related files. While adding the new codeowner to specific paths is done correctly, the introduction of broad wildcard patterns (*rocm*, *quark*, *aiter*) is problematic. These patterns override more specific ownership rules defined earlier in the file, which would unintentionally remove existing codeowners from files they should be reviewing. I've left a critical comment explaining the issue and suggesting a fix.
.github/CODEOWNERS
Outdated
| *rocm* @tjtanaa | ||
| *quark* @tjtanaa | ||
| *aiter* @tjtanaa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These broad wildcard patterns are problematic because they override the more specific ownership rules defined on lines 108-113. According to GitHub's documentation, the last matching pattern in the CODEOWNERS file determines the owners.
For example, the rule *rocm* @tjtanaa on line 114 will make @tjtanaa the sole owner for any path containing rocm, including those on lines 108-113. This removes @gshtras as a codeowner for those files, which is likely not the intended behavior.
The same issue applies to *aiter*, which would override ownership for files like vllm/model_executor/layers/fused_moe/rocm_aiter_fused_moe.py.
To fix this, please remove these broad patterns. If you need to add @tjtanaa as a codeowner for other directories (like requirements, tests, docs as mentioned in the PR description), you should add more specific rules for them, for example:
/requirements/*rocm* @tjtanaa
/tests/**/*rocm* @tjtanaa
/docs/**/*rocm* @tjtanaa
/vllm/**/*quark* @tjtanaa
/tests/**/*quark* @tjtanaa
/docs/**/*quark* @tjtanaa
/vllm/**/*aiter* @tjtanaa
/tests/**/*aiter* @tjtanaa
For now, I'll suggest removing these lines to fix the immediate ownership issue. You can then add the more specific paths in a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
.github/CODEOWNERS
Outdated
| # ROCm related: specify owner with write access to notify AMD folks for careful code review | ||
| /docker/Dockerfile.rocm* @gshtras | ||
| /vllm/v1/attention/backends/rocm*.py @gshtras | ||
| /vllm/v1/attention/backends/mla/rocm*.py @gshtras | ||
| /vllm/attention/ops/rocm*.py @gshtras | ||
| /vllm/model_executor/layers/fused_moe/rocm*.py @gshtras | ||
| /docker/Dockerfile.rocm* @gshtras @tjtanaa | ||
| /vllm/v1/attention/backends/rocm*.py @gshtras @tjtanaa | ||
| /vllm/v1/attention/backends/mla/rocm*.py @gshtras @tjtanaa | ||
| /vllm/attention/ops/rocm*.py @gshtras @tjtanaa | ||
| /vllm/model_executor/layers/fused_moe/rocm*.py @gshtras @tjtanaa | ||
| /csrc/rocm @gshtras @tjtanaa | ||
| *rocm* @tjtanaa | ||
| *quark* @tjtanaa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep existing ROCm reviewers when adding wildcard
In CODEOWNERS the last matching rule wins, so the newly added *rocm* @tjtanaa wildcard overrides the more specific ROCm patterns above that list @gshtras as a reviewer. Any file whose name contains rocm will now request only @tjtanaa, defeating the preceding comment about notifying AMD folks and silently removing @gshtras from review for every ROCm path. Either the wildcard should include the previous owners or it should appear before the specific entries to avoid dropping current coverage.
Useful? React with 👍 / 👎.
Signed-off-by: tjtanaa <tunjian.tan@embeddedllm.com>
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds @tjtanaa as a codeowner for ROCm and multi-modal related files. The changes are mostly correct and align with the PR's intent. I have one suggestion to use a more general glob pattern for ROCm files under the /vllm directory to ensure comprehensive coverage and simplify the CODEOWNERS file, which seems to be the original intent.
| /vllm/v1/attention/backends/rocm*.py @gshtras @tjtanaa | ||
| /vllm/v1/attention/backends/mla/rocm*.py @gshtras @tjtanaa | ||
| /vllm/attention/ops/rocm*.py @gshtras @tjtanaa | ||
| /vllm/model_executor/layers/fused_moe/rocm*.py @gshtras @tjtanaa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To better align with the PR's goal of covering all ROCm-related files and to simplify the CODEOWNERS file, consider using a single glob pattern for all rocm files under the /vllm directory. This would also ensure future ROCm-related files are automatically covered, which seems to be part of your intent as stated in the PR description. The current changes only add ownership to existing specific paths.
/vllm/**/*rocm* @gshtras @tjtanaa
youkaichao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
welcome!
youkaichao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait until @tjtanaa accepts github invite
Signed-off-by: tjtanaa <tunjian.tan@embeddedllm.com>
|
@youkaichao Thank you. I have accepted the invite. |
DarkLight1337
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GH shows that the CODEOWNERS file is valid now
|
Congratulations! |
Signed-off-by: tjtanaa <tunjian.tan@embeddedllm.com> Signed-off-by: xuebwang-amd <xuebwang@amd.com>
Signed-off-by: tjtanaa <tunjian.tan@embeddedllm.com>
Purpose
Thank you very much for the opportunity. I would also like to help out on multimodality to ensure its stable support on ROCm.
To covers all ROCm related features,
the following pattern is used:The above has been modified based on gemini's advice. I have used a more specific paths:
They cover the following existing patterns
and potential new files.
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.