Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds CI enforcement for the fastdeploy/envs.py file by requiring approval from specific reviewers when this file is modified. However, the PR also includes what appears to be test code in the form of a new environment variable with a _test suffix.
Key Changes:
- Adds approval check in CI for modifications to
fastdeploy/envs.py - Adds a new environment variable
FD_PREFILL_WAIT_DECODE_RESOURCE_SECONDS_test(appears to be test code)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| scripts/check_approval.sh | Adds CI check requiring approval from yuanlehome, rainyfly, or Wanglongzhi2001 when fastdeploy/envs.py is modified |
| fastdeploy/envs.py | Adds a new environment variable with _test suffix, which appears to be test code rather than a production feature |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| "FD_ENABLE_PDL": lambda: int(os.getenv("FD_ENABLE_PDL", "1")), | ||
| # "Number of tokens in the group for Mixture of Experts (MoE) computation processing on HPU" | ||
| "FD_HPU_CHUNK_SIZE": lambda: int(os.getenv("FD_HPU_CHUNK_SIZE", "64")), | ||
| "FD_PREFILL_WAIT_DECODE_RESOURCE_SECONDS": lambda: int(os.getenv("FD_PREFILL_WAIT_DECODE_RESOURCE_SECONDS", "30")), |
There was a problem hiding this comment.
新增的环境变量缺少注释说明。文件中其他环境变量都有注释来说明其用途（参见第151-152行的 FD_HPU_CHUNK_SIZE 示例）。请添加注释来说明这个环境变量的用途。
建议在第154行之前添加类似这样的注释：
# Description of what this environment variable does| "FD_PREFILL_WAIT_DECODE_RESOURCE_SECONDS": lambda: int(os.getenv("FD_PREFILL_WAIT_DECODE_RESOURCE_SECONDS", "30")), | |
| "FD_PREFILL_WAIT_DECODE_RESOURCE_SECONDS": lambda: int(os.getenv("FD_PREFILL_WAIT_DECODE_RESOURCE_SECONDS", "30")), | |
| # Timeout in seconds to wait for decode resource during prefill (test mode) |
fastdeploy/envs.py
Outdated
| "FD_PREFILL_WAIT_DECODE_RESOURCE_SECONDS_test": lambda: int( | ||
| os.getenv("FD_PREFILL_WAIT_DECODE_RESOURCE_SECONDS_test", "30") | ||
| ), |
There was a problem hiding this comment.
这个环境变量名包含 _test 后缀，看起来像是测试代码。如果这是用于CI测试的临时变量，不应该提交到主分支。如果这是一个正式的环境变量，应该使用更清晰和专业的命名，避免使用 _test 后缀。
建议移除此变量，或者如果确实需要，请使用更有意义的名称来说明其用途。
| "FD_PREFILL_WAIT_DECODE_RESOURCE_SECONDS_test": lambda: int( | |
| os.getenv("FD_PREFILL_WAIT_DECODE_RESOURCE_SECONDS_test", "30") | |
| ), |
|
|
||
| ENV_FILE="fastdeploy/envs.py" | ||
|
|
||
| HAS_ENV_MODIFY=$(git diff upstream/$BRANCH --name-only | grep -E "^${ENV_FILE}$" || true) |
There was a problem hiding this comment.
BRANCH is expanded unquoted in the shell command git diff upstream/$BRANCH --name-only, which allows command injection if an attacker can influence the branch name (e.g., a PR from a fork with a branch name containing ; or backticks). This can lead to arbitrary command execution in CI with your secrets (like GITHUB_TOKEN). Quote the variable to prevent the shell from interpreting metacharacters, e.g.,
HAS_ENV_MODIFY=$(git diff "upstream/${BRANCH}" --name-only | grep -E "^${ENV_FILE}$" || true)Additionally consider validating BRANCH against a safe pattern (e.g., ^[A-Za-z0-9._/-]+$).
|
Thanks for your contribution! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5331 +/- ##
==========================================
Coverage ? 59.18%
==========================================
Files ? 324
Lines ? 39932
Branches ? 6033
==========================================
Hits ? 23635
Misses ? 14436
Partials ? 1861
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for your contribution! |
3 participants
Motivation
Add env CI to make envs more clear.
Modifications
Add env ci, pr need to get approval of specific RD to pass CI.
Usage or Command
None.
Accuracy Tests
None.
Checklist
[FDConfig],[APIServer],[Engine],[Scheduler],[PD Disaggregation],[Executor],[Graph Optimization],[Speculative Decoding],[RL],[Models],[Quantization],[Loader],[OP],[KVCache],[DataProcessor],[BugFix],[Docs],[CI],[Optimization],[Feature],[Benchmark],[Others],[XPU],[HPU],[GCU],[DCU],[Iluvatar],[Metax]]pre-commitbefore commit.releasebranch, make sure the PR has been submitted to thedevelopbranch, then cherry-pick it to thereleasebranch with the[Cherry-Pick]PR tag.