Repository navigation
lint: enforce G004 (lazy %-format logging) with temporary ignores for un-migrated code (#5118) - #5125
Merged
Merged
Conversation
Enable ruff's G004 so an f-string in a logging call is rejected, and add per-file-ignores for the files that are not migrated to %-format yet; entries can be deleted one at a time as the migration continues. ruff matches per-file-ignores patterns with '*' crossing directory separators, so 'lmcache/*.py' would ignore the whole package subtree. Measured: a naive per-directory list also ignores 771 already-migrated files and leaves only 494 of 1337 files protected. The list is therefore generated from the actual violations: '<dir>/**' only where every file below the directory violates the rule, an explicit file entry otherwise. Signed-off-by: Runguo LI <[email protected]>
LiRunGuo
requested review from
ApostaC,
deng451e,
hickeyma and
sammshen
as code owners
September 15, 2026 12:52
maobaolong
approved these changes
Sep 15, 2026
maobaolong
left a comment
Collaborator
There was a problem hiding this comment.
@LiRunGuo LGTM. Thanks for this work!
chunxiaozheng
approved these changes
Sep 17, 2026
chunxiaozheng
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for your contribution, LGTM!
This was referenced Sep 18, 2026
This was referenced Sep 18, 2026
This was referenced Sep 18, 2026
2 tasks
This was referenced Sep 19, 2026
2 tasks
This was referenced Sep 21, 2026
2 tasks
2 tasks
neevmodh
added a commit
to neevmodh/LMCache
that referenced
this pull request
Sep 23, 2026
Follow-up for LMCache#5118 after LMCache#5125. Converts the remaining G004 f-string logging calls in benchmarks/rag/rag.py and benchmarks/rag/utils.py to lazy %-style logging arguments, and removes the now-unneeded per-file G004 ignores from pyproject.toml. Signed-off-by: Neev Modh <[email protected]>
maobaolong
pushed a commit
that referenced
this pull request
Sep 23, 2026
Follow-up for #5118 after #5125. Converts the remaining G004 f-string logging calls in benchmarks/rag/rag.py and benchmarks/rag/utils.py to lazy %-style logging arguments, and removes the now-unneeded per-file G004 ignores from pyproject.toml. Signed-off-by: Neev Modh <[email protected]>
maobaolong
added a commit
that referenced
this pull request
Sep 23, 2026
) Removes two stale per-file ignore entries from [tool.ruff.lint.per-file-ignores]: - lmcache/v1/storage_backend/connector/infinistore_adapter.py (clean since #5125) - lmcache/v1/storage_backend/connector/redis_adapter.py (clean since #5125) Both files pass ruff check --isolated --select G004 without the ignore entry. Refs #5205 Signed-off-by: Yunare Maia <[email protected]> Co-authored-by: maobaolong <[email protected]>
This was referenced Sep 23, 2026
Closed
This was referenced Oct 14, 2026
maobaolong
pushed a commit
that referenced
this pull request
Oct 22, 2026
#5167) [good-first-issue] distributed: convert f-string log calls to %-format (#5118) Follow-up step for the G004 rollout in #5118. #5125 enabled G004 repo-wide with temporary per-file-ignores for the not-yet-migrated files, and the plan agreed there is to remove those entries directory by directory as each area is cleaned up. This does that for lmcache/v1/distributed/, the last two files under it that were still ignored: - l2_adapters/bigtable_l2_adapter.py (5 call sites) - transfer_channel/impl/mooncake_te_impl.py (4 call sites) and deletes both per-file-ignores entries, so the directory can no longer regress. Together with lmcache/v1/multiprocess/ (already clean), both directories that enforce SLF now enforce G004 as well. Format style only, no behaviour change. Format specifiers were chosen from each variable's type annotation per docs/coding_standards.md 7.4: %d for the int timestamps, sizes, byte limits, port and batch id; %s for the str keys, column name, session id, host, protocol and device; %.2f for the float second-diff. All nine messages render byte-identically to the f-strings they replace. get_rpc_port() is intentionally left as a call in the argument list: lazy formatting defers interpolation, not argument evaluation, so this keeps the existing behaviour rather than making an unrelated change. Part of #5118 Signed-off-by: sanjayrohith <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Refs #5118
Refs #3372
Enables ruff's
G004("logging statement uses f-string") and adds temporaryper-file-ignoresfor the files that have not been migrated to lazy%-formatlogging yet — the first step requested by @maobaolong in #5118.
loggingrenders a record lazily asmsg % args, so an f-string pays for itsformatting even when the level is disabled. Today nothing prevents a new
logger.info(f"...")from landing in a file that was already migrated, which iswhy this keeps generating one-file good-first-issue PRs: 15+ such PRs have been
merged since March 2026 (#5014, #5094, #4970, #4785, #4727, #4650, #4479, #4357,
#4290, #4142, #4124, #4102, #3977, #3975, #3973) and #4937, #4506, #5106, #5110,
#5112 are still open.
Why the ignore list is mostly per file
Ruff matches
per-file-ignorespatterns with*crossing directoryseparators, so
"lmcache/*.py" = ["G004"]silently ignores the whole packagesubtree. A naive per-directory list (
lmcache/*.py,lmcache/v1/*.py,lmcache/v1/storage_backend/*.py, ...) would additionally ignore 771already-migrated files, leaving only 494 of 1337 lintable files protected —
including
lmcache/v1/multiprocess/andlmcache/v1/distributed/, the twodirectories that already enforce SLF.
So the list is derived from the actual violations:
"<dir>/**" = ["G004"]only where every file below that directory violatesthe rule — 4 entries:
benchmarks/storage_backend_io,examples/disagg_prefill,examples/disagg_prefill_mp,examples/token_dropping/rkv_variants;Result: exactly the 72 violating files are ignored and no clean file is (checked
programmatically), while the rule guards the other 1265 files. Entries can be
deleted one by one as migration continues; each one is either a single file or a
directory that is entirely un-migrated.
Notebooks are covered too —
examples/token_dropping/*.ipynbandexamples/token_dropping/rkv_variants/**account for 41 of the 223 violations.Verification
ruff check .→All checks passed!,ruff check --statistics .→ no findings.logger.info(f"..."),deleted again afterwards):
lmcache/v1/multiprocess/<file>,lmcache/v1/storage_backend/plugins/<file>(a subdirectory of an un-migrateddirectory) and
lmcache/v1/distributed/<file>were all reported asG004,while the ignored files stayed quiet.
clean files.
SKIP=rust-fmt,rust-clippy pre-commit run --all-files→ all hooks pass(
cargois not installed in my environment).Special notes for your reviewers:
pyproject.toml; no runtime code is touched.G201(24 sites) andG002(1 site) are deliberately not enabled: they changewhat is logged rather than when it is formatted, so they deserve a separate
decision (noted in [Proposal] Enforce lazy %-format logging (ruff G004) so the f-string logging migration stops regressing #5118).
fit for new contributors, as you suggested.
If applicable: