-
Notifications
You must be signed in to change notification settings - Fork 7.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tests for error codes with local API server #3131
base: main
Are you sure you want to change the base?
Conversation
to test model info. In future commits will test the other endpoints with a model installed. Signed-off-by: Adam Treat <treat.adam@gmail.com>
Signed-off-by: Adam Treat <treat.adam@gmail.com>
Signed-off-by: Adam Treat <treat.adam@gmail.com>
Signed-off-by: Jared Van Bortel <jared@nomic.ai>
Signed-off-by: Jared Van Bortel <jared@nomic.ai>
Signed-off-by: Jared Van Bortel <jared@nomic.ai>
file(DOWNLOAD | ||
"${TEST_MODEL_URL}" | ||
"${TEST_MODEL_PATH}" | ||
EXPECTED_HASH "MD5=${TEST_MODEL_MD5}" | ||
) |
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.
This is going to be downloaded by default for anyone building GPT4All with Python installed, at configure time. If we expect this to be shared by the C++ and Python tests, I would prefer if it was downloaded as the first step of the check
target, before it runs ctest.
configure_file(python/config.py.in "${CMAKE_CURRENT_SOURCE_DIR}/python/config.py") | ||
|
||
add_test(NAME ChatPythonTests | ||
COMMAND ${Python3_EXECUTABLE} -m pytest --color=yes "${CMAKE_CURRENT_SOURCE_DIR}/python" | ||
COMMAND ${Python3_EXECUTABLE} -m pytest -s --color=yes "${CMAKE_CURRENT_SOURCE_DIR}/python" |
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 can imagine wanting to add -s via the PYTEST_ADDOPTS env var at the console if you are trying to debug a test that e.g. succeeds when it should fail, but why should the default be to disable capturing of output? This is going to make successful test runs quite verbose.
@pytest.fixture | ||
def chat_server_with_model(chat_server_with_model_config: dict[str, str]) -> Iterator[None]: | ||
chat_executable = Path(os.environ['CHAT_EXECUTABLE']).absolute() | ||
with subprocess.Popen(chat_executable, env=chat_server_with_model_config) as process: | ||
try: | ||
yield | ||
except: | ||
process.kill() | ||
raise | ||
process.send_signal(signal.SIGINT) | ||
if retcode := process.wait(): | ||
raise CalledProcessError(retcode, process.args) |
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.
There is no need for the code duplication between chat_server_config/chat_server_with_model_config and chat_server/chat_server_with_model.
There only really need to be two fixtures: chat_server and chat_server_with_model. They can each be a single line function that calls some common function like def prepare_chat_server(with_model: bool = False)
. You can always return
a generator, or yield from
if it's called inside of a with
block.
with pytest.raises(HTTPError) as excinfo: | ||
request.get('foobarbaz', wait=True) | ||
assert excinfo.value.response.status_code == 404 |
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.
For checks like this I would prefer to pass an argument like raise=False
that disables the raise_for_status call, and returns a resp.status_code, resp.json()
tuple (typed as tuple[int, Any]
).
Also, it wouldn't hurt to test the response body, even for non-200 responses.
assert excinfo.value.response.status_code == 404 | ||
|
||
# empty model list | ||
response = request.get('models', wait=True) |
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.
wait=True was intended for only the first request in a test, as the server is still starting. Subsequent requests should not need to retry.
assert 'choices' in response | ||
assert response['choices'][0]['text'] == ' jumps over the lazy dog.' |
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.
We can be more strict here, at least by checking the number of choices and the keys in a choice:
assert 'choices' in response | |
assert response['choices'][0]['text'] == ' jumps over the lazy dog.' | |
assert len(response['choices']) == 1 | |
assert response['choices'][0].keys() == {'text', 'index', 'logprobs', 'finish_reason'} | |
assert response['choices'][0]['text'] == ' jumps over the lazy dog.' |
pytest.xfail('This causes an assertion failure in the app. See https://github.com/nomic-ai/gpt4all/issues/3133') | ||
with pytest.raises(HTTPError) as excinfo: | ||
response = request.post('completions', data=data, wait=True) |
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 believe this asserts that the entire test fails, not just this part. Checks that are expected to fail should be split into their own test functions.
local_file_path_env = os.getenv('TEST_MODEL_PATH') | ||
if local_file_path_env: | ||
local_file_path = Path(local_file_path_env) | ||
if local_file_path.exists(): | ||
shutil.copy(local_file_path, app_data_dir / local_file_path.name) | ||
else: | ||
pytest.fail(f'Model file specified in TEST_MODEL_PATH does not exist: {local_file_path}') | ||
else: | ||
pytest.fail('Environment variable TEST_MODEL_PATH is not set') |
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.
It's better to ask for forgiveness than for permission (in Python, at least). This could be simply:
local_file_path_env = os.getenv('TEST_MODEL_PATH') | |
if local_file_path_env: | |
local_file_path = Path(local_file_path_env) | |
if local_file_path.exists(): | |
shutil.copy(local_file_path, app_data_dir / local_file_path.name) | |
else: | |
pytest.fail(f'Model file specified in TEST_MODEL_PATH does not exist: {local_file_path}') | |
else: | |
pytest.fail('Environment variable TEST_MODEL_PATH is not set') | |
shutil.copy(os.environ['TEST_MODEL_PATH'], app_data_dir / local_file_path.name) |
And an informative KeyError or FileNotFoundError will be raised if there is a problem. This also clarifies that this is not something being tested (pass/fail), but an expectation about the environment the test itself runs in.
Add tests for error codes with local API server and also install a model to test model info. In future commits will test the other endpoints with a model installed.