Skip to content

lint: enforce G004 (lazy %-format logging) with temporary ignores for un-migrated code (#5118) - #5125

Merged
chunxiaozheng merged 1 commit into
LMCache:devfrom
LiRunGuo:gfi/5118-enforce-g004
Sep 17, 2026
Merged

chunxiaozheng merged 1 commit into
LMCache:devfrom
LiRunGuo:gfi/5118-enforce-g004

Conversation

@LiRunGuo

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:
Refs #5118
Refs #3372

Enables ruff's G004 ("logging statement uses f-string") and adds temporary
per-file-ignores for the files that have not been migrated to lazy %-format
logging yet — the first step requested by @maobaolong in #5118.

logging renders a record lazily as msg % args, so an f-string pays for its
formatting even when the level is disabled. Today nothing prevents a new
logger.info(f"...") from landing in a file that was already migrated, which is
why 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-ignores patterns with * crossing directory
separators
, so "lmcache/*.py" = ["G004"] silently ignores the whole package
subtree. A naive per-directory list (lmcache/*.py, lmcache/v1/*.py,
lmcache/v1/storage_backend/*.py, ...) would additionally ignore 771
already-migrated files
, leaving only 494 of 1337 lintable files protected —
including lmcache/v1/multiprocess/ and lmcache/v1/distributed/, the two
directories that already enforce SLF.

So the list is derived from the actual violations:

  • "<dir>/**" = ["G004"] only where every file below that directory violates
    the rule — 4 entries: benchmarks/storage_backend_io, examples/disagg_prefill,
    examples/disagg_prefill_mp, examples/token_dropping/rkv_variants;
  • an explicit file entry otherwise — 66 entries.

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/*.ipynb and
examples/token_dropping/rkv_variants/** account for 41 of the 223 violations.

Verification

  • ruff check . → All checks passed!, ruff check --statistics . → no findings.
  • Enforcement sentinel (three temporary files containing logger.info(f"..."),
    deleted again afterwards): lmcache/v1/multiprocess/<file>,
    lmcache/v1/storage_backend/plugins/<file> (a subdirectory of an un-migrated
    directory) and lmcache/v1/distributed/<file> were all reported as G004,
    while the ignored files stayed quiet.
  • Ignore-set audit: the 70 entries cover all 72 violating files and ignore zero
    clean files.
  • SKIP=rust-fmt,rust-clippy pre-commit run --all-files → all hooks pass
    (cargo is not installed in my environment).

Special notes for your reviewers:

If applicable:

  • this PR contains user facing changes - docs added
  • this PR contains unit tests

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]>

@maobaolong maobaolong left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LiRunGuo LGTM. Thanks for this work!

@chunxiaozheng chunxiaozheng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution, LGTM!

@chunxiaozheng
chunxiaozheng enabled auto-merge (squash) September 17, 2026 03:26
@github-actions github-actions Bot added the full Run comprehensive tests on this PR label Sep 17, 2026
@chunxiaozheng
chunxiaozheng merged commit f48dbd0 into LMCache:dev Sep 17, 2026
35 of 36 checks passed
This was referenced Sep 18, 2026
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]>
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

full Run comprehensive tests on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants