Fix EP_BUFFER_DEBUG=0 turning on Python-side debug output - #719
Conversation
`EP_BUFFER_DEBUG` is documented in the README as a `0`/`1` toggle that defaults to `0`, and the C++ side honors that by parsing the value as an int via `get_env<int>` before testing it. The two Python call sites tested the raw environment string instead. Since the string `'0'` is truthy in Python, setting `EP_BUFFER_DEBUG=0` — the documented way to disable the flag — enabled the Python `print` calls while the C++ half stayed correctly silent. Convert the value before testing it, matching `get_env<int>` and the idiom already used by every other boolean env flag in the package (`EP_SUPPRESS_NCCL_CHECK`, `EP_USE_NVIDIA_TOOLS`, `EP_DISABLE_BARRIER_PROFILING`, `EP_REUSE_NCCL_COMM`). Fixes deepseek-ai#718 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| @@ -308,7 +308,7 @@ def __init__(self, | |||
| num_max_tokens_per_rank, hidden, num_topk, use_fp8_dispatch, | |||
There was a problem hiding this comment.
🟡 warning: 非本次 PR 引入的既有 lint 问题(在未修改的 origin/main @ 01dc3aa 上同样存在,PR 描述中已附对比数据):L6 存在 F401(typing.List 未使用),L930/L1088 存在 F541(无占位符的 f-string),另有约 259 行 yapf 格式差异。PR 选择不在此内清理以避免淹没 2 行修复,此取舍合理、已如实说明;若希望该文件达到 lint-clean,建议另开独立 PR 处理(PR 描述中也已提出此意愿)。
🤖 v4f
There was a problem hiding this comment.
Confirmed, and agreed on keeping it out of this PR.
To put numbers on it for anyone reading later — measured with the pinned tooling (yapf==0.40.2, ruff==0.6.5, repo pyproject.toml) against unmodified origin/main (01dc3aa) versus this branch:
origin/main this PR
yapf --diff lines 259 259
ruff errors 3 3
Identical burden, so this PR adds zero new violations. The 3 ruff errors are F401 (typing.List unused, L6) and F541 x2 (f-strings without placeholders, L930/L1088).
The root cause of why these never surface on main is that format.sh lints only files differing from origin/main, so main never lints this file at all — meaning format-check goes red for any PR that touches elastic.py, regardless of content.
I'll open the cleanup as a separate PR whenever a maintainer confirms they want it. Worth noting #685 is blocked by the same thing, so one cleanup PR would unblock format-check for both.
| allow_hybrid_mode, allow_multiple_reduction) | ||
|
|
||
| if os.environ.get('EP_BUFFER_DEBUG', 0): | ||
| if int(os.environ.get('EP_BUFFER_DEBUG', 0)): |
There was a problem hiding this comment.
🔵 suggestion: Non-blocking edge case: with int() a non-numeric value (e.g. EP_BUFFER_DEBUG=true or an empty string) now raises ValueError at init time instead of being silently truthy/falsy. This matches the existing idiom for every other EP_* flag and roughly mirrors C++ sscanf behavior, so it's fine as-is; if you ever harden env parsing package-wide, a small shared helper (e.g. env_flag(name)) could centralize this for all EP* boolean flags.
🤖 v5
There was a problem hiding this comment.
Verified the edge case — it's real, and slightly broader than true/empty. Everything below was previously silently truthy and now raises ValueError:
| value | before | after |
|---|---|---|
| unset | off | off |
0, 00 |
on | off |
1, 2, -1, " 1 " |
on | on |
"" |
off | ValueError |
" " |
on | ValueError |
true, TRUE, false, off |
on | ValueError |
0x1, 1abc |
on | ValueError |
On the sscanf comparison
One correction worth recording: it isn't parity. sscanf doesn't fail loudly — it returns 0/-1 and leaves the output operand untouched, and get_env<int> declares int value; uninitialized (csrc/utils/system.hpp:29), so an unparseable value returns an indeterminate int. Compiling the same cases with a poisoned initial value:
0 -> ret=1 parsed=0 -> false
1 -> ret=1 parsed=1 -> true
(empty) -> ret=-1 NOT PARSED (value left 0xDEADBEEF -> UB)
true -> ret=0 NOT PARSED (value left 0xDEADBEEF -> UB)
0x1 -> ret=1 parsed=0 -> false # stops at 'x'
1abc -> ret=1 parsed=1 -> true
So on EP_BUFFER_DEBUG=true the C++ side reads uninitialized memory and silently picks whatever truthiness falls out, while Python now raises immediately. Python-after is stricter than C++ on 0x1/1abc and louder on true/empty. Both seem preferable to UB, but they are not the same behavior, so I don't want the PR record claiming they match.
(Separately: that uninitialized int value; looks worth a fix on the C++ side independent of this PR — happy to file it if you agree it's a real issue.)
Why I'm keeping the bare int()
Consistency with the sibling flags is the entire basis of this fix, and all six of them raise identically on these inputs today — EP_SUPPRESS_NCCL_CHECK (deep_ep/__init__.py:51), EP_USE_NVIDIA_TOOLS (testing.py:143), EP_DISABLE_BARRIER_PROFILING (testing.py:152), EP_REUSE_NCCL_COMM (comm.py:62), EP_NUM_MAX_LOCAL_RANKS (elastic.py:288), and setup.py's DISABLE_* (:130, :153). Hardening only EP_BUFFER_DEBUG would make it the odd one out and trade one inconsistency for another.
Agreed that _env_flag() is the right home for this, and that it should cover all of them at once rather than ride along in a 2-line bugfix. Glad to send it as a follow-up — the open question for maintainers is whether a non-numeric value should raise, warn-and-default, or accept true/false/on/off, since that's a user-facing contract change wider than this fix.
🤖 ds-review-bot Code Reviewv6改动正确修复了 v4f该 PR 修复了 EP_BUFFER_DEBUG=0 时 Python 侧调试输出被错误打开的问题(Fixes #718)。根因是 deep_ep/buffers/elastic.py 的 :311 与 :828 两处用原始环境变量字符串做真值测试,而 Python 中 '0' 为真值,导致显式设置为 0(README 文档化的关闭方式)时 Python 侧 print 仍输出,与 C++ 侧(csrc/utils/system.hpp 的 get_env<int> 通过 sscanf("%d") 解析后测试)行为不一致。修复方式为测试前转换为 int:if int(os.environ.get('EP_BUFFER_DEBUG', 0)):,与包内其他布尔环境变量(EP_SUPPRESS_NCCL_CHECK、EP_USE_NVIDIA_TOOLS、EP_DISABLE_BARRIER_PROFILING、EP_REUSE_NCCL_COMM)的既有写法保持一致。改动仅 2 行(1 文件,2 insertions / 2 deletions),且都位于守卫 print 调用的 if 条件内,不影响其他代码路径;未设置环境变量时默认值 0 仍生效,因此 unset 路径与启用(1/2)路径行为完全不变,仅 '0' 这一行从 on 改为 off,与 C++ 侧在全部取值上达成一致。已逐项核对::311 与 :828 两处均为 int() 转换后的测试;包内 grep 确认不存在未转换的 EP_BUFFER_DEBUG 读取;git diff HEAD~1 HEAD 确认变更范围(1 文件、2+/2-)与描述一致;工作树干净。修复正确、范围最小、语义与 C++ 侧及 README 文档一致,审查通过。唯一提示:deep_ep/buffers/elastic.py 存在主分支即有的 lint 问题(非本 PR 引入),详见 comments。 v5Correct, minimal 2-line bugfix. Both Python call sites in deep_ep/buffers/elastic.py (lines 311 and 828) now wrap os.environ.get('EP_BUFFER_DEBUG', 0) in int(), so the documented 'off' value EP_BUFFER_DEBUG=0 no longer enables Python-side debug prints. Verified: (1) the diff matches the description exactly — 2 lines, both if-conditions guarding print calls; (2) semantics now match C++ get_env<int> (csrc/utils/system.hpp, sscanf %d) for unset/0/1/2 — only the '0' row changes behavior; (3) grep confirms these were the only raw-string boolean env reads in the package, and all other EP_* flags already use the int(os.environ.get(...)) idiom; (4) csrc/kernels/backend/nccl.cu:70's get_env("EP_BUFFER_DEBUG", 0) deduces dtype_t=int from the literal 0, so the C++ side needs no change; (5) the red format-check job is pre-existing lint identical on origin/main (01dc3aa) — 259 yapf diff lines and 3 ruff errors (F401 at L6, F541 x2 at L930/L1088) — and this PR introduces zero new violations; deferring the ~259-line reformat to a separate PR is the right call. Files reviewed: 1 |
Fixes #718.
Problem
EP_BUFFER_DEBUG=0— the value the README documents as "off" — turns the Python-side debug output on, while the C++ side correctly stays off.The README documents the flag as a
0/1toggle defaulting to0:The C++ side honors that contract by converting before testing (
csrc/kernels/backend/nccl.cu:38,:45,:79;csrc/elastic/buffer.hpp:865,:1055):get_env<int>runs the value throughsscanf(c_str, "%d", &value)(csrc/utils/system.hpp:28-31), so"0"becomes0— falsy.The two Python call sites tested the raw string instead, and
'0'is truthy in Python:The
intdefault0only applies when the variable is absent, which is why the unset case works and only the explicitly-disabled case misbehaves.Fix
Convert before testing, matching
get_env<int>and the idiom already used by every other boolean env flag in the package:Existing call sites that already do this —
deep_ep/__init__.py:51(EP_SUPPRESS_NCCL_CHECK),deep_ep/utils/testing.py:143(EP_USE_NVIDIA_TOOLS),:152(EP_DISABLE_BARRIER_PROFILING),deep_ep/utils/comm.py:62(EP_REUSE_NCCL_COMM), andsetup.py:130/:153. The two lines changed here were the only boolean env reads in the package that omitted the conversion.Verification
Host-side; the defect and the fix are both pure Python and need no device. Evaluating the exact expression at
elastic.py:311/:828before and after:EP_BUFFER_DEBUGget_env<int>)012Python now agrees with the C++ side on every value. Only the
0row changes; the unset and enabled paths are untouched.Scope
Two lines, both inside
ifconditions guardingprintcalls. No behavior change to any code path other than the one that was contradicting its own documentation.Note on the
format-checkjobCI is red here on pre-existing lint in
deep_ep/buffers/elastic.py, not on anything this PR introduces.format.shonly lints files that differ fromorigin/main, somainnever lints this file and these never surface there.Running the pinned tooling (
yapf==0.40.2,ruff==0.6.5, repopyproject.toml) against unmodifiedorigin/main(01dc3aa) versus this branch:Identical — this PR adds zero new violations. The 3 ruff errors on
mainareF401(typing.Listunused, L6) andF541×2 (f-strings without placeholders, L930/L1088).I deliberately did not fold that cleanup in: the yapf delta alone is a ~259-line reformat, which would bury a 2-line bugfix. Happy to send it as a separate PR if you'd like the file made lint-clean — I notice #685 ran into the same thing and made the same offer.