[CLI] Implement lmcache ping subcommand#2859
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces the lmcache ping subcommand, complete with a design document, implementation, and corresponding tests. The overall implementation is well-structured and aligns with the provided design. My review focuses on a few areas for improvement related to code modularity, style guide adherence, and test robustness. Specifically, I've suggested moving a shared utility function to a dedicated module to improve architecture, refactoring a helper function to enhance code clarity, and strengthening tests to more thoroughly validate behavior. I've also pointed out a minor style guide violation regarding import placement.
|
|
||
| # First Party | ||
| from lmcache.cli.commands.base import BaseCommand | ||
| from lmcache.cli.commands.describe import normalize_url |
There was a problem hiding this comment.
To improve modularity and avoid coupling between CLI command modules, the normalize_url function should be moved from describe.py to a shared utility file (e.g., lmcache/cli/utils.py). The design document docs/design/cli/ping.md also suggests this as a preferred option. This would make normalize_url reusable by other commands in the future without creating dependencies between them.
References
- The style guide emphasizes architectural soundness and modularity. Creating a dependency between two command modules (
pinganddescribe) for a shared utility function goes against this principle. (link)
| start = time.monotonic() | ||
| try: | ||
| with urllib.request.urlopen( | ||
| urllib.request.Request(url), timeout=timeout | ||
| ) as resp: | ||
| rtt_ms = (time.monotonic() - start) * 1000 | ||
| if resp.status == 200: | ||
| return ("OK", rtt_ms, None) | ||
| return ("FAIL", rtt_ms, f"HTTP {resp.status}") | ||
| except urllib.error.HTTPError as exc: | ||
| rtt_ms = (time.monotonic() - start) * 1000 | ||
| return ("FAIL", rtt_ms, f"HTTP {exc.code}: {exc.reason}") | ||
| except (urllib.error.URLError, OSError) as exc: | ||
| rtt_ms = (time.monotonic() - start) * 1000 | ||
| reason = getattr(exc, "reason", str(exc)) | ||
| return ("FAIL", rtt_ms, f"Cannot connect to {url}: {reason}") |
There was a problem hiding this comment.
The calculation of rtt_ms is repeated in the try block and in each except block. This can be refactored to remove code duplication and improve readability by calculating the round-trip time once after the try...except block.
| start = time.monotonic() | |
| try: | |
| with urllib.request.urlopen( | |
| urllib.request.Request(url), timeout=timeout | |
| ) as resp: | |
| rtt_ms = (time.monotonic() - start) * 1000 | |
| if resp.status == 200: | |
| return ("OK", rtt_ms, None) | |
| return ("FAIL", rtt_ms, f"HTTP {resp.status}") | |
| except urllib.error.HTTPError as exc: | |
| rtt_ms = (time.monotonic() - start) * 1000 | |
| return ("FAIL", rtt_ms, f"HTTP {exc.code}: {exc.reason}") | |
| except (urllib.error.URLError, OSError) as exc: | |
| rtt_ms = (time.monotonic() - start) * 1000 | |
| reason = getattr(exc, "reason", str(exc)) | |
| return ("FAIL", rtt_ms, f"Cannot connect to {url}: {reason}") | |
| start = time.monotonic() | |
| status, error = "OK", None | |
| try: | |
| with urllib.request.urlopen( | |
| urllib.request.Request(url), timeout=timeout | |
| ) as resp: | |
| if resp.status != 200: | |
| status, error = "FAIL", f"HTTP {resp.status}" | |
| except urllib.error.HTTPError as exc: | |
| status, error = "FAIL", f"HTTP {exc.code}: {exc.reason}" | |
| except (urllib.error.URLError, OSError) as exc: | |
| reason = getattr(exc, "reason", str(exc)) | |
| status, error = "FAIL", f"Cannot connect to {url}: {reason}" | |
| rtt_ms = (time.monotonic() - start) * 1000 | |
| return status, rtt_ms, error | |
References
- The style guide emphasizes code cleanliness. Refactoring to remove duplicated logic improves maintainability and readability. (link)
| # Standard | ||
| from unittest.mock import patch |
There was a problem hiding this comment.
The import from unittest.mock import patch is located inside the test_ok method. According to the repository style guide, imports should be grouped at the top of the file. Please move this import to the Standard import section at the top of the file. This also applies to the same import in TestPingCommandEngine.test_ok on line 170.
References
- According to the repository style guide, imports should be grouped at the top of the file in the order: Standard, Third Party, First Party, Local. (link)
| with pytest.raises(SystemExit): | ||
| cmd.execute(FakeArgs()) |
There was a problem hiding this comment.
This test correctly verifies that SystemExit is raised, but it doesn't confirm that the command attempted to connect to the correct default URL. The test can be strengthened by capturing stderr and asserting that the error message contains the expected default URL (http://localhost:8080/api/healthcheck). You'll need to ensure from unittest.mock import patch and import io are at the top of the file.
| with pytest.raises(SystemExit): | |
| cmd.execute(FakeArgs()) | |
| with pytest.raises(SystemExit), patch( | |
| "sys.stderr", new_callable=io.StringIO | |
| ) as mock_stderr: | |
| cmd.execute(FakeArgs()) | |
| assert "http://localhost:8080/api/healthcheck" in mock_stderr.getvalue() |
References
- The style guide states that tests should verify the public interface and docstring contract. The current test does not fully verify the contract that a specific default URL is used. (link)
| with pytest.raises(SystemExit): | ||
| cmd.execute(FakeArgs()) |
There was a problem hiding this comment.
Similar to test_kvcache_default, this test should be strengthened to verify that the correct default URL for the engine (http://localhost:8000/health) was used. This can be done by capturing stderr and checking the contents of the error message. You'll need to ensure from unittest.mock import patch and import io are at the top of the file.
| with pytest.raises(SystemExit): | |
| cmd.execute(FakeArgs()) | |
| with pytest.raises(SystemExit), patch( | |
| "sys.stderr", new_callable=io.StringIO | |
| ) as mock_stderr: | |
| cmd.execute(FakeArgs()) | |
| assert "http://localhost:8000/health" in mock_stderr.getvalue() |
References
- The style guide states that tests should verify the public interface and docstring contract. The current test does not fully verify the contract that a specific default URL is used. (link)
fix Signed-off-by: Oasis-Git <ayw.sirius19@gmail.com> Co-authored-by: Kuntai Du <kuntai@uchicago.edu>
fix Signed-off-by: Oasis-Git <ayw.sirius19@gmail.com> Co-authored-by: Kuntai Du <kuntai@uchicago.edu>
fix Signed-off-by: Oasis-Git <ayw.sirius19@gmail.com> Co-authored-by: Kuntai Du <kuntai@uchicago.edu>
fix Signed-off-by: Oasis-Git <ayw.sirius19@gmail.com> Co-authored-by: Kuntai Du <kuntai@uchicago.edu>
What this PR does / why we need it:
Special notes for your reviewers:
If applicable: