feat: add meaningful LMStudioPredictionError subclasses and session-s…#136
feat: add meaningful LMStudioPredictionError subclasses and session-s…#136RajaMuhammadAwais wants to merge 3 commits intolmstudio-ai:mainfrom
Conversation
…coped model loading fixture
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
| @@ -0,0 +1,70 @@ | |||
| # This workflow will upload a Python Package to PyPI when a release is created | |||
There was a problem hiding this comment.
We have a publish.yml workflow, no need for this one.
| @@ -0,0 +1,10 @@ | |||
| { | |||
There was a problem hiding this comment.
Editor specific config shouldn't be added to the repo.
(I have .vscode in my user level ~/.gitignore, but perhaps we should add it to the repo level one as well).
| from lmstudio import AsyncClient, EmbeddingLoadModelConfig, LMStudioModelNotFoundError | ||
|
|
||
| from ..support import ( | ||
| from tests.support import ( |
There was a problem hiding this comment.
The use of relative imports is intentional and shouldn't be changed.
| class LMStudioPredictionError(LMStudioServerError): | ||
| """Problems reported by the LM Studio instance during a model prediction.""" | ||
|
|
||
| @sdk_public_type |
There was a problem hiding this comment.
The more granular subclasses and the session scoped fixture loading would be easier to follow if placed in different PRs.
| from tests.load_models import reload_models | ||
| asyncio.run(reload_models()) | ||
| except Exception as e: | ||
| print(f"[Fixture] Skipping model loading: {e}", file=sys.stderr) |
There was a problem hiding this comment.
Some environments consider printing anything to stderr to be a test failure in its own right, so ideally we wouldn't even try to load the models when not lmstudio is part of the test marker filtering (or all the other model using test cases are otherwise filtered out).
This suggests that rather than autouse=True, we want something along the lines of the "marker to fixture usage" approach suggested in the this pytest issue comment:
def pytest_itemcollected(item):
if item.get_closest_marker("lmstudio") is not None:
item.applymarker(pytest.mark.usefixtures("load_required_models"))
| @sdk_public_type | ||
|
|
There was a problem hiding this comment.
| @sdk_public_type |
Duplicated decorator
| "lmstudio: marks tests as needing LM Studio (deselect with '-m \"not lmstudio\"')", | ||
| "wip: marks tests as a work-in-progress (select with '-m \"wip\"')" | ||
| "wip: marks tests as a work-in-progress (select with '-m \"wip\"')", | ||
| "asyncio: marks tests as asyncio-based", |
There was a problem hiding this comment.
| "asyncio: marks tests as asyncio-based", |
pytest.mark.asyncio is considered a standard marker (as long as pytest-asyncio is installed in the test execution environment), so it's allowed without needing to be explicitly listed even when strict markers are enabled.
| from .json_api import ( | ||
| LMStudioPredictionError, | ||
| LMStudioModelLoadError, | ||
| LMStudioInputValidationError, | ||
| LMStudioPredictionTimeoutError, | ||
| LMStudioPredictionCancelledError, | ||
| LMStudioPredictionRuntimeError, | ||
| ) |
There was a problem hiding this comment.
| from .json_api import ( | |
| LMStudioPredictionError, | |
| LMStudioModelLoadError, | |
| LMStudioInputValidationError, | |
| LMStudioPredictionTimeoutError, | |
| LMStudioPredictionCancelledError, | |
| LMStudioPredictionRuntimeError, | |
| ) |
Exporting symbols at the top level is handled by listing them in the relevant __all__ list (the one in json_api in this case)
ncoghlan
left a comment
ncoghlan
left a comment
There was a problem hiding this comment.
Thanks for taking a first pass at this. Three big items:
- the session scoped test fixture and the more detailed exceptions should be separate PRs
- defining the more detailed exceptions is only the first step, the more significant step is raising them from the relevant locations (potentially narrowing the expected exception type in affected test cases)
- there are several cosmetic changes which should be omitted entirely
Additional details on those points inline.
The CLA will also need to be signed before a PR can be accepted.
2 participants
…coped model loading fixture