Update_testing #15
Update_testing #15
Conversation
- Improve LiteLLM endpoint implementation with additional features - Add comprehensive test suite for LiteLLM endpoint (499 lines) - Expand experiments and plotting functionality - Enhance test coverage across runner, experiments, and plotting modules - Update utility functions and MLflow callback tests
- Apply ruff formatting and fix linting issues across codebase - Fix unused variable assignments in tests - Update import re-exports in endpoints/__init__.py - Use tmp_path fixture for test file generation to prevent repository pollution - Add test_file.json* to .gitignore to exclude generated test files
athewsey
force-pushed
the
update_testing
branch
from
7aff0bc to
093da4f
Compare
|
(Rebased following merge of the small fix in #13) |
athewsey
left a comment
athewsey
left a comment
There was a problem hiding this comment.
I'm seeing some test failures on this since I merged #13 (😅) but hopefully these should be pretty isolated & easy to fix with just expecting write_html instead.
A few questions raised which it'd be good to address, but apart from those it's looking good IMO
| else: | ||
| # If it's not a BaseException, wrap it in an ImportError | ||
| self.exc = ImportError(str(exception)) |
There was a problem hiding this comment.
Although we use it only for ImportErrors so far, this class was originally written to be generic. The need for this if/else is arising from inconsistent usage across LLMeter:
- In some cases e.g.
plotlyinplotting.py, we use it as originally documented - to store aModuleNotFoundError/ImportError. - In others e.g.
kaleidoinplotting.py, we pass in a message string instead.
My asks would probably be to:
- Standardize our usage one way or the other, unless we have good reasons not to, and
- If we still want to support both string and error in here, then either
- Create a more generic base class like
Exceptionin the else clause, OR - Change the name and docstring of this
DeferredError(e.g.DeferredImportError?) to indicate that it's only intended for import errors.
- Create a more generic base class like
| @@ -1,0 +1,2 @@ | |||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
For me, ruff format picks up & adds the missing trailing newline in this and tests/endpoint/__init__.py... Does it not for you?
| from unittest.mock import call, Mock, patch | ||
| from unittest.mock import Mock, call, patch | ||
|
|
||
| # External Dependencies: | ||
| import boto3 | ||
| from moto import mock_aws | ||
| import pytest | ||
| from moto import mock_aws |
There was a problem hiding this comment.
Any particular reason to un-alphabetize these imports? The call vs Mock one I could understand depending how the formatter treats different cases, but if I revert these changes Ruff doesn't ask to apply either of them in my environment?
| pass | ||
| response.num_tokens_input = None | ||
| response.num_tokens_output = None |
There was a problem hiding this comment.
Looking at the InvocationResponse definition, these fields should default to None anyway. Why do we need to set them here?
| return InvocationResponse.error_output( | ||
| id=uuid4().hex, error=str(e), input_prompt=self._parse_payload(payload) | ||
| response = InvocationResponse.error_output( | ||
| input_payload=payload, error=e, id=uuid4().hex |
There was a problem hiding this comment.
Looks like runner.py, endpoints/bedrock.py, endpoints/openai.py, and endpoints/sagemaker.py all still have some cases using error=str(e) - do we care enough to fix that consistently and add an Exception | str | None type annotation to InvocationResponse.error_output()'s definition?
| existing_options = kwargs.get("stream_options", {}) | ||
| payload_copy["stream_options"] = {**existing_options, "include_usage": True} |
There was a problem hiding this comment.
Should we merge in case there are some stream_options defined in payload as well as the kwargs? Currently we're supporting passing in either way, but not mixing.
Could be e.g.
existing_kwargs_options = kwargs["stream_options"]
existing_payload_options = payload_copy.get("stream_options", {})
payload_copy["stream_options"] = {
**existing_payload_options,
**existing_kwargs_options,
"include_usage": True,
}2 participants
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.