-
Notifications
You must be signed in to change notification settings - Fork 2.9k
[ci] fix: fix precommit #4760
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
base: main
Are you sure you want to change the base?
[ci] fix: fix precommit #4760
Conversation
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 addresses pre-commit hook failures, primarily through code formatting adjustments. The changes align the code with the project's style guidelines. I've provided one suggestion to refactor a lengthy, repetitive list in a test configuration. Using list multiplication will make the code more concise, readable, and easier to maintain, especially if the number of layers changes in the future.
| "sliding_attention", | ||
| "full_attention", | ||
| "sliding_attention", | ||
| "full_attention", | ||
| "sliding_attention", | ||
| "full_attention", | ||
| "sliding_attention", | ||
| "full_attention", | ||
| "sliding_attention", | ||
| "full_attention", | ||
| "sliding_attention", | ||
| "full_attention", | ||
| "sliding_attention", | ||
| "full_attention", | ||
| "sliding_attention", | ||
| "full_attention", | ||
| "sliding_attention", | ||
| "full_attention", | ||
| "sliding_attention", | ||
| "full_attention", | ||
| "sliding_attention", | ||
| "full_attention", | ||
| "sliding_attention", | ||
| "full_attention", |
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.
This long list of repeating strings can be made much more concise and maintainable by using list multiplication. This approach makes the code easier to read and simplifies future modifications if the number of layers changes.
| "sliding_attention", | |
| "full_attention", | |
| "sliding_attention", | |
| "full_attention", | |
| "sliding_attention", | |
| "full_attention", | |
| "sliding_attention", | |
| "full_attention", | |
| "sliding_attention", | |
| "full_attention", | |
| "sliding_attention", | |
| "full_attention", | |
| "sliding_attention", | |
| "full_attention", | |
| "sliding_attention", | |
| "full_attention", | |
| "sliding_attention", | |
| "full_attention", | |
| "sliding_attention", | |
| "full_attention", | |
| "sliding_attention", | |
| "full_attention", | |
| "sliding_attention", | |
| "full_attention", | |
| *["sliding_attention", "full_attention"] * 12, |
What does this PR do?
Checklist Before Starting
[{modules}] {type}: {description}(This will be checked by the CI){modules}includefsdp,megatron,sglang,vllm,rollout,trainer,ci,training_utils,recipe,hardware,deployment,ray,worker,single_controller,misc,perf,model,algo,env,tool,ckpt,doc,data,cfg,reward,like[megatron, fsdp, doc]{type}is infeat,fix,refactor,chore,test[BREAKING]to the beginning of the title.[BREAKING][fsdp, megatron] feat: dynamic batchingTest
API and Usage Example
# Add code snippet or script demonstrating how to use thisDesign & Code Changes
Checklist Before Submitting
Important
Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.
pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=alwaysci-requestchannel in theverlSlack workspace. (If not accessible, please try the Feishu group (飞书群).)