-
Notifications
You must be signed in to change notification settings - Fork 383
Snapshot record then reply testing #740
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,8 +1,22 @@ | ||||||||
| from typing import override | ||||||||
| import functools | ||||||||
| import inspect | ||||||||
| import json | ||||||||
| import os | ||||||||
| from collections.abc import Callable, Coroutine | ||||||||
| from typing import Any, override | ||||||||
| from unittest.mock import patch | ||||||||
| from urllib import parse | ||||||||
|
|
||||||||
| import vcr | ||||||||
| from vcr.config import RecordMode | ||||||||
| from vcr.request import Request | ||||||||
|
|
||||||||
| from splunklib.ai.model import PredefinedModel | ||||||||
| from tests.ai_test_model import InternalAIModel, TestLLMSettings, create_model | ||||||||
| from tests.testlib import SDKTestCase | ||||||||
|
|
||||||||
| REDACTED_APP_KEY = "[[[--APPKEY-REDACTED-]]]" | ||||||||
|
|
||||||||
|
|
||||||||
| class AITestCase(SDKTestCase): | ||||||||
| _model: PredefinedModel | None = None | ||||||||
|
|
@@ -42,3 +56,147 @@ async def model(self) -> PredefinedModel: | |||||||
| model = await create_model(self.test_llm_settings) | ||||||||
| self._model = model | ||||||||
| return model | ||||||||
|
|
||||||||
|
|
||||||||
| def ai_snapshot_test() -> Callable[ | ||||||||
| [Callable[..., Coroutine[Any, Any, None]]], Callable[..., Coroutine[Any, Any, None]] | ||||||||
| ]: | ||||||||
| def decorator( | ||||||||
| fn: Callable[..., Coroutine[Any, Any, None]], | ||||||||
| ) -> Callable[..., Coroutine[Any, Any, None]]: | ||||||||
| source_file = inspect.getfile(fn) | ||||||||
| test_dir = os.path.dirname(source_file) | ||||||||
| test_file = os.path.splitext(os.path.basename(source_file))[0] | ||||||||
|
|
||||||||
| snapshot_dir = os.path.join(test_dir, "snapshots", test_file) | ||||||||
| snapshot_filename = f"{fn.__qualname__}.json" | ||||||||
|
|
||||||||
| @functools.wraps(fn) | ||||||||
| async def wrapper(self: AITestCase, *args: Any, **kwargs: Any) -> None: | ||||||||
| settings = self.test_llm_settings | ||||||||
| assert settings.internal_ai is not None | ||||||||
|
|
||||||||
| internal_ai_hostname = parse.urlparse( | ||||||||
| settings.internal_ai.base_url | ||||||||
| ).hostname | ||||||||
| assert internal_ai_hostname is not None | ||||||||
|
|
||||||||
| class _JSONFriendlySerializer: | ||||||||
| def deserialize(self, serialized: str) -> Any: | ||||||||
| assert settings.internal_ai is not None | ||||||||
| serialized = serialized.replace( | ||||||||
| REDACTED_APP_KEY, settings.internal_ai.app_key | ||||||||
| ) | ||||||||
|
|
||||||||
| data = json.loads(serialized) | ||||||||
| for interaction in data.get("interactions", []): | ||||||||
| interaction["request"]["uri"] = interaction["request"][ | ||||||||
| "uri" | ||||||||
| ].replace("internal-ai-host", internal_ai_hostname, 1) | ||||||||
|
|
||||||||
| interaction["request"]["body"] = json.dumps( | ||||||||
| interaction["request"]["body"] | ||||||||
| ) | ||||||||
| body = interaction["response"]["body"] | ||||||||
| interaction["response"]["body"] = {} | ||||||||
| interaction["response"]["body"]["string"] = json.dumps(body) | ||||||||
|
|
||||||||
| return data | ||||||||
|
|
||||||||
| def serialize(self, dict: Any) -> str: | ||||||||
| for interaction in dict.get("interactions", []): | ||||||||
| interaction["request"]["uri"] = interaction["request"][ | ||||||||
| "uri" | ||||||||
| ].replace(internal_ai_hostname, "internal-ai-host", 1) | ||||||||
|
|
||||||||
| body = interaction["request"]["body"] | ||||||||
| interaction["request"]["body"] = json.loads(body) | ||||||||
|
|
||||||||
| resp_body = interaction["response"]["body"]["string"] | ||||||||
| interaction["response"]["body"] = json.loads(resp_body) | ||||||||
|
|
||||||||
| out = json.dumps(dict, indent=4) + "\n" | ||||||||
| assert settings.internal_ai is not None | ||||||||
| out = out.replace(settings.internal_ai.app_key, REDACTED_APP_KEY) | ||||||||
|
|
||||||||
| # Assert that nothing is leaking into the public snapshots. | ||||||||
| assert internal_ai_hostname not in out.lower() | ||||||||
| assert settings.internal_ai.app_key.lower() not in out.lower() | ||||||||
| assert settings.internal_ai.base_url.lower() not in out.lower() | ||||||||
| assert settings.internal_ai.token_url.lower() not in out.lower() | ||||||||
| assert settings.internal_ai.client_id.lower() not in out.lower() | ||||||||
| assert settings.internal_ai.client_secret.lower() not in out.lower() | ||||||||
|
|
||||||||
| return out | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. curious if we should look for the other secrets for our testing LLM before serializing - to make sure we do not leak anything by mistake when creating new snapshots. or add some ci stage for that. WDYT?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do you have anything other in mind to check? We are only recording LLM http requests, and i think we are checking everything. |
||||||||
|
|
||||||||
| def _before_record_request(request: Request) -> Request | None: | ||||||||
| url = parse.urlparse(request.uri) # pyright: ignore[reportUnknownArgumentType, reportUnknownVariableType] | ||||||||
| if url.hostname == internal_ai_hostname: | ||||||||
| request.headers = {} | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we keep some specific headers for like Authorization and keep comparing them?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought that it makes sense, but now i realized that that would only test our test suite since the header that is used for our internal ai does not match how our end used will use this, say with OpenAI API directly. splunk-sdk-python/tests/ai_test_model.py Lines 49 to 51 in b1219b2
I don't believe now that the complexity of adding this is worth. |
||||||||
| return request | ||||||||
| return None | ||||||||
|
|
||||||||
| def _before_record_response(response: Any) -> Any: | ||||||||
| response["headers"] = {} | ||||||||
| return response | ||||||||
|
|
||||||||
| def _json_body_matcher(r1: Any, r2: Any) -> None: | ||||||||
| b1 = json.loads(r1.body) | ||||||||
| b2 = json.loads(r2.body) | ||||||||
| if b1 != b2: | ||||||||
| raise AssertionError(f"Body mismatch:\n{b1}\n!=\n{b2}") | ||||||||
|
|
||||||||
| my_vcr = vcr.VCR( | ||||||||
| cassette_library_dir=snapshot_dir, | ||||||||
| serializer="json-friendly", | ||||||||
| record_mode=RecordMode.ONCE, | ||||||||
| match_on=[ | ||||||||
| "method", | ||||||||
| "scheme", | ||||||||
| "host", | ||||||||
| "port", | ||||||||
| "path", | ||||||||
| "query", | ||||||||
| "jsonbody", | ||||||||
| ], | ||||||||
| before_record_request=_before_record_request, | ||||||||
| before_record_response=_before_record_response, | ||||||||
| # record_on_exception=False, | ||||||||
| # drop_unused_requests=True, | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to be removed if not needed
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to keep them since I have found them as useful options to toggle when you are adding say a new test or just experimenting with this recordings, They are not used now but i uncommented them multiple times, so might be worth, WDYT? |
||||||||
| ) | ||||||||
| my_vcr.register_serializer("json-friendly", _JSONFriendlySerializer()) | ||||||||
| my_vcr.register_matcher("jsonbody", _json_body_matcher) | ||||||||
|
|
||||||||
| with my_vcr.use_cassette(snapshot_filename): # pyright: ignore[reportGeneralTypeIssues] | ||||||||
| await fn(self, *args, **kwargs) | ||||||||
|
|
||||||||
| return wrapper | ||||||||
|
|
||||||||
| return decorator | ||||||||
|
|
||||||||
|
|
||||||||
| def deterministic_thread_ids() -> Callable[ | ||||||||
|
mateusz834 marked this conversation as resolved.
|
||||||||
| [Callable[..., Coroutine[Any, Any, None]]], Callable[..., Coroutine[Any, Any, None]] | ||||||||
| ]: | ||||||||
| def decorator( | ||||||||
| fn: Callable[..., Coroutine[Any, Any, None]], | ||||||||
| ) -> Callable[..., Coroutine[Any, Any, None]]: | ||||||||
| @functools.wraps(fn) | ||||||||
| async def wrapper(self: AITestCase, *args: Any, **kwargs: Any) -> None: | ||||||||
| counter = 0 | ||||||||
|
|
||||||||
| def _deterministic_uuid() -> str: | ||||||||
| nonlocal counter | ||||||||
| result = f"00000000-0000-0000-0000-{counter:012d}" | ||||||||
| counter += 1 | ||||||||
| return result | ||||||||
|
|
||||||||
| with patch( | ||||||||
| "splunklib.ai.engines.langchain._thread_id_new_uuid", | ||||||||
| side_effect=_deterministic_uuid, | ||||||||
| ): | ||||||||
| await fn(self, *args, **kwargs) | ||||||||
|
|
||||||||
| return wrapper | ||||||||
|
|
||||||||
| return decorator | ||||||||
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.
[nitpick] the name suggests it can be used only for AI. What do you think about record_snapshot name?
Uh oh!
There was an error while loading. Please reload this page.
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 do filtering here, so i think it only works for AI 😄 .