-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Convert llm-load-test to package-able format #78
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good step towards modularizing the tool, I'll give it a try locally.
# pdm stores project-wide configurations in .pdm.toml, but it is recommended to not include it | ||
# in version control. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this file need to be generated locally to install all the required dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pulled those lines from the Github Python gitignore template; AFAIK any PDM configuration can be done through the pyproject.toml
file.
However we probably won't need any build system configuration since our build process does not require any external library support.
8bdc296
to
b88d7f7
Compare
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe pull request introduces a comprehensive restructuring of the llm-load-test project, focusing on improving package organization, dependency management, and project structure. The changes include creating a Changes
Assessment against linked issues
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/llm_load_test/load_test.py (2)
16-46
: Consider using monotonic time for more robust timing.In
run_main_process
, you rely ontime.time()
, which can jump if the system clock changes. For load testing, usingtime.monotonic()
is often safer and less error-prone.- start_time = time.time() - current_time = start_time - while (current_time - start_time) < duration: + start_time = time.monotonic() + current_time = start_time + while current_time - start_time < duration:
153-157
: Add test coverage for main concurrency loop.The loop in
main
that adjusts concurrency (for n_users in concurrency:
) is central to the tool’s functionality. Consider adding or expanding automated tests to ensure the logic handles edge cases (e.g., concurrency set to [0], concurrency set to very large values) without issues.Would you like me to propose a new test file or add tests inline to an existing suite?
tox.ini (1)
32-32
: Consider re-enabling pylint to maintain consistent code quality.Pylint is commented out in
[testenv:linters]
. Having both flake8 and pylint active would help catch a wider range of issues. Plan to re-enable it in your follow-up PR to ensure thorough lint coverage.README.md (1)
27-32
: Ensure usage instructions remain consistent.The README usage lines reference two different commands:
python load_test.py
andload-test
. Consider aligning them for clarity so users see a single, consistent command form.-Run the tool with the following command: `python load_test.py -c config.yaml` +Run the tool with the following command: `load-test -c config.yaml`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pdm.lock
is excluded by!**/*.lock
📒 Files selected for processing (13)
.gitignore
(1 hunks)README.md
(3 hunks)load_test.py
(1 hunks)pyproject.toml
(1 hunks)requirements.txt
(1 hunks)src/llm_load_test/generation_pb2_grpc.py
(2 hunks)src/llm_load_test/load_test.py
(1 hunks)src/llm_load_test/plugins/dummy_plugin.py
(1 hunks)src/llm_load_test/plugins/hf_tgi_plugin.py
(1 hunks)src/llm_load_test/plugins/openai_plugin.py
(1 hunks)src/llm_load_test/plugins/tgis_grpc_plugin.py
(1 hunks)src/llm_load_test/utils.py
(1 hunks)tox.ini
(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- src/llm_load_test/utils.py
- requirements.txt
- src/llm_load_test/generation_pb2_grpc.py
- src/llm_load_test/plugins/openai_plugin.py
- src/llm_load_test/plugins/dummy_plugin.py
- src/llm_load_test/plugins/hf_tgi_plugin.py
- src/llm_load_test/plugins/tgis_grpc_plugin.py
- load_test.py
- pyproject.toml
🧰 Additional context used
🪛 GitHub Actions: linters
src/llm_load_test/load_test.py
[warning] 10-12: Import statements are in wrong order and missing proper grouping
🔇 Additional comments (5)
.gitignore (2)
88-96
: LGTM! PDM configuration looks good.The PDM-related ignore patterns align with the project's transition to a packageable format:
.pdm.toml
: Correctly ignored as it contains project-wide configurations.pdm-python
: Correctly ignored as it's environment-specific.pdm-build/
: Correctly ignored as it contains build artifactsThe configuration follows PDM's best practices and includes helpful documentation links.
89-90
: Consider tracking pdm.lock in version control.The comment suggests including
pdm.lock
in version control, but it's currently commented out. The lock file helps ensure reproducible builds by pinning exact versions of dependencies. This is particularly important for a load testing tool where consistent behavior across environments is crucial.Run this script to check if pdm.lock exists and contains dependency information:
✅ Verification successful
Uncomment pdm.lock in .gitignore to track it in version control
The pdm.lock file exists and contains critical dependency information including exact versions, hashes, and requirements. For a load testing tool, tracking this file ensures reproducible builds and consistent behavior across environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if pdm.lock exists and verify its contents if [ -f "pdm.lock" ]; then echo "pdm.lock exists. Contents:" cat pdm.lock else echo "pdm.lock not found. Consider creating it with 'pdm lock'" fiLength of output: 40820
src/llm_load_test/load_test.py (1)
160-170
: Confirm safe termination of subprocesses.The
KeyboardInterrupt
and genericException
handlers both callstop_procs
andstop_test
. Verify that user processes don’t leave behind dangling threads or objects when an exception occurs mid-test. If needed, add final cleanup or a robust teardown to ensure consistent resource deallocation.tox.ini (1)
26-26
: Consider cross-platform compatibility for linting.The command uses
bash -c 'find src/ -name "*.py"'
, which may fail on non-Unix systems lackingbash
. If you wish to support Windows, consider a platform-agnostic approach like a Python one-liner or adding an equivalent script.README.md (1)
7-7
: Clarify Python version requirements.You’ve updated the requirement to “Python 3.10 or newer.” Ensure that your
pyproject.toml
and any CI configuration also reflect 3.10 as the minimum version so that users aren’t misled.
from llm_load_test.dataset import Dataset | ||
|
||
from llm_load_test import logging_utils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolve import ordering to match linter rules.
The linter warns that these imports are not grouped or ordered properly. Typically, we group imports by standard library, third-party libraries, and local modules. Try consolidating the local modules in one block to satisfy style guidelines.
Apply the following diff to reorder imports and fix the pipeline warning:
import logging
import logging.handlers
import multiprocessing as mp
import sys
import time
-from llm_load_test.user import User
-from llm_load_test.dataset import Dataset
-
from llm_load_test import logging_utils
from llm_load_test import utils
+
+from llm_load_test.dataset import Dataset
+from llm_load_test.user import User
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: linters
[warning] 10-12: Import statements are in wrong order and missing proper grouping
Caution An unexpected error occurred while opening a pull request: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
@coderabbitai Please propose some unit tests |
Tip For best results, initiate chat on the files or code changes. @sjmonson Here's a proposal for unit tests. I'll break them down into multiple test files: # tests/test_load_test.py
import pytest
from unittest.mock import Mock, patch
from multiprocessing import Queue
from llm_load_test.load_test import run_main_process, gather_results, stop_procs, create_procs
def test_run_main_process():
# Mock dependencies
dataset = Mock()
dataset.get_next_n_queries.return_value = ["query1", "query2"]
dataset_q = Queue()
stop_q = Queue()
# Test normal execution
with patch('time.time') as mock_time, patch('time.sleep'):
mock_time.side_effect = [0, 0.5, 1, 1.5, 2] # Simulate time progression
run_main_process(2, 1, dataset, dataset_q, stop_q)
# Verify dataset queue was filled
assert dataset_q.qsize() == 0 # Queue should be empty after cleanup
assert stop_q.qsize() == 1 # Stop signal should be sent
def test_gather_results():
# Mock pipes
pipe1, pipe2 = Mock(), Mock()
pipe1.recv.return_value = ["result1", "result2"]
pipe2.recv.return_value = ["result3"]
results = gather_results([pipe1, pipe2])
assert len(results) == 3
assert "result1" in results
assert "result3" in results
def test_stop_procs():
# Mock processes and queues
proc1, proc2 = Mock(), Mock()
dataset_q = Queue()
stop_q = Queue()
# Add some items to queues
dataset_q.put("item1")
stop_procs([proc1, proc2], dataset_q, stop_q)
# Verify cleanup
assert proc1.join.called
assert proc2.join.called
assert dataset_q.empty()
assert stop_q.empty() # tests/test_plugins.py
import pytest
from llm_load_test.plugins.dummy_plugin import DummyPlugin
from llm_load_test.result import RequestResult
def test_dummy_plugin_request():
# Test non-streaming request
plugin = DummyPlugin({"streaming": False})
query = {
"text": "hello world",
"output_tokens": 5,
"input_tokens": 2
}
result = plugin.request_http(query, user_id=1)
assert isinstance(result, RequestResult)
assert result.output_text == "dlrow olleh" # Reversed text
assert result.output_tokens == 5
def test_dummy_plugin_streaming():
# Test streaming request
plugin = DummyPlugin({"streaming": True})
query = {
"text": "hello world",
"output_tokens": 5,
"input_tokens": 2
}
result = plugin.streaming_request_http(query, user_id=1)
assert isinstance(result, RequestResult)
assert result.ack_time is not None
assert result.first_token_time is not None
assert result.end_time > result.start_time # tests/test_result.py
import pytest
from llm_load_test.result import RequestResult
def test_request_result_calculation():
result = RequestResult(user_id=1, input_id="test1", input_tokens=10)
result.start_time = 1000
result.ack_time = 1001
result.first_token_time = 1002
result.end_time = 1005
result.output_tokens = 20
result.calculate_results()
assert result.time_to_first_token == 2 # 1002 - 1000
assert result.total_time == 5 # 1005 - 1000
assert result.tokens_per_second == 4 # 20 tokens / 5 seconds
def test_request_result_error_handling():
result = RequestResult(user_id=1, input_id="test1", input_tokens=10)
result.error_code = 500
result.error_text = "Internal Server Error"
assert result.has_error()
assert result.error_code == 500 # tests/test_integration.py
import pytest
from llm_load_test.load_test import main
from llm_load_test.plugins.dummy_plugin import DummyPlugin
@pytest.fixture
def mock_config(tmp_path):
config_file = tmp_path / "test_config.yaml"
config_file.write_text("""
plugin: "dummy"
plugin_options:
streaming: false
dataset:
type: "simple"
input_text: "test query"
input_tokens: 2
output_tokens: 5
load_options:
concurrency: 2
duration: 1
""")
return str(config_file)
def test_complete_flow(mock_config):
with pytest.raises(SystemExit) as exc_info:
main(["-c", mock_config, "--log_level", "debug"])
assert exc_info.value.code == 0 These tests cover:
Additional tests should be added for:
Would you like me to generate tests for any specific component in more detail? 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script:
Length of output: 33540 ✏️ Learnings added
|
Seems ok... would have to try these tests |
This PR converts the llm-load-test repo into a more packaging friendly format.
src/llm_load_test
.requirements.txt
with apyproject.toml
as per python packaging guidelines.load_test.py
script to the project root that calls the main entrypoint of the package for legacy compatibility.Note: This PR fixes an issue with the linting config where certain files were missed and thus there are many new linting errors.
Partially Implements #76
Closes #75
Closes #77
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores
pyproject.toml
Build