Add generated Lex Runtime V2 client with integration tests#51
Add generated Lex Runtime V2 client with integration tests#51Alan4506 wants to merge 2 commits intoawslabs:developfrom
Conversation
| @@ -0,0 +1,4 @@ | |||
| { | |||
There was a problem hiding this comment.
Do we need this changelog entry? Our release automation should handle new client setup, including initial versioning and changelog entries. We will have merge conflicts if the wording if different or we may have repeated entries because the uuid in the newly generated entry is different.
There was a problem hiding this comment.
Same comment as before. This should be done by our release automation.
| @@ -0,0 +1,10 @@ | |||
| # Code generated by smithy-python-codegen DO NOT EDIT. | |||
There was a problem hiding this comment.
Anyway we can avoid this header on the package README? This will also appear on the PyPI page for the package.
| return bot_id, bot_alias_id, locale_id | ||
|
|
||
|
|
||
| def _wait_for_bot(lex, bot_id: str, timeout: int = 60) -> None: |
There was a problem hiding this comment.
We should use our waiters instead of a custom wait function. Waiters poll AWS resources until they become available so it's meant for cases like these. According to the boto3 docs, we have a bot_available waiter that waits until a bot is available after its created. We can probably do something like this:
response = lex.create_bot(
botName=bot_name,
roleArn=role_arn,
dataPrivacy={"childDirected": False},
idleSessionTTLInSeconds=300,
)
bot_id = response["botId"]
print(f"Created bot: {bot_id}")
lex.get_waiter("bot_available").wait(
botId=bot_id,
WaiterConfig=WAITER_CONFIG,
)| raise TimeoutError("Bot did not become available") | ||
|
|
||
|
|
||
| def _wait_for_bot_locale( |
There was a problem hiding this comment.
Same here, we should be should be using waiters.
I see this functions handles two cases: 1) checking that the locale is created and 2) checking that it's built. These two cases should be handled by the bot_locale_created and bot_locale_built waiters.
| BOT_ALIAS_ID = "TSTALIASID" | ||
| LOCALE_ID = "en_US" |
There was a problem hiding this comment.
nit: We are setting these up again instead of using the output from setup_resources.py which can introduce some drift.
|
|
||
| dependencies = [ | ||
| "smithy_aws_core[eventstream, json]~=0.4.0", | ||
| "smithy_core~=0.3.0", |
There was a problem hiding this comment.
When I pulled down these changes, installed the client locally, and tried to run the integ tests, I got this error:
TypeError: APIOperation.__init__() got an unexpected keyword argument 'error_schemas'
This is because the error_schemas field was introduced in smithy-core 0.4.0 which was released on Tuesday. However, the codegen changes to generate these were merged as part of this commit on 3/31.
I think this client was created in between the actual release of smithy-core and the codegen merge. Let's regenerate the client using the latest release to make sure all dependencies are up-to-date for testing.
| from aws_sdk_lex_runtime_v2.client import LexRuntimeV2Client | ||
| from aws_sdk_lex_runtime_v2.config import Config | ||
|
|
||
| BOT_ID = os.environ["LEX_BOT_ID"] |
There was a problem hiding this comment.
I prefer if we retrieved the env variables in the tests themselves with error handling for cases where the user did not set up the resources.
Right now if I run these tests without setup I get:
================================== ERRORS ==================================
____ ERROR collecting tests/integration/test_bidirectional_streaming.py ____
../../../../../.local/share/uv/python/cpython-3.12.9-macos-aarch64-none/lib/python3.12/importlib/__init__.py:90: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
<frozen importlib._bootstrap>:1387: in _gcd_import
???
<frozen importlib._bootstrap>:1360: in _find_and_load
???
<frozen importlib._bootstrap>:1310: in _find_and_load_unlocked
???
<frozen importlib._bootstrap>:488: in _call_with_frames_removed
???
<frozen importlib._bootstrap>:1387: in _gcd_import
???
<frozen importlib._bootstrap>:1360: in _find_and_load
???
<frozen importlib._bootstrap>:1331: in _find_and_load_unlocked
???
<frozen importlib._bootstrap>:935: in _load_unlocked
???
<frozen importlib._bootstrap_external>:999: in exec_module
???
<frozen importlib._bootstrap>:488: in _call_with_frames_removed
???
tests/integration/__init__.py:11: in <module>
BOT_ID = os.environ["LEX_BOT_ID"]
^^^^^^^^^^^^^^^^^^^^^^^^
<frozen os>:714: in __getitem__
???
E KeyError: 'LEX_BOT_ID'
========================= short test summary info ==========================
ERROR tests/integration/test_bidirectional_streaming.py - KeyError: 'LEX_BOT_ID'
!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!
============================= 1 error in 0.15s =============================
The error is not handled cleanly and the user has to look for where LEX_BOT_ID is set, which isn't in the test.
If I gracefully handle it in the test similar to our transcribe streaming tests:
================================= FAILURES =================================
_________________________ test_start_conversation __________________________
async def test_start_conversation() -> None:
"""Test bidirectional streaming StartConversation operation."""
client = create_lex_client("us-east-1")
bot_id = os.environ.get("LEX_BOT_ID")
if not bot_id:
> pytest.fail("LEX_BOT_ID environment variable not set")
E Failed: LEX_BOT_ID environment variable not set
tests/integration/test_bidirectional_streaming.py:98: Failed
========================= short test summary info ==========================
FAILED tests/integration/test_bidirectional_streaming.py::test_start_conversation - Failed: LEX_BOT_ID environment variable not set
============================ 1 failed in 0.13s =============================
The failure point is clear and easier to debug.
There was a problem hiding this comment.
Thanks for the great write-up here, Antonio :)
While it has it's merits, personally I don't love the idea of having environment variables required for our integ tests at all. If we start adding a bunch of these, they can get out of hand really quickly. It's easy to imagine a scenario where we have
LEX_BOT_ID, but also LEX_BOT_S3_OUTPUT_BUCKET, which is different from TRANSCRIBE_STREAMING_OUTPUT_S3_BUCKET, and just have a mounting list of resources that we all have to make and set in our environment before running the test.
These tests should be self-contained whenever possible. Have we discussed before what our philosophy is for when we should do each of the following?
- Add a new resource via environment variable (as is done in this PR)
- Skip an integ test entirely
- Mock service responses instead of relying on AWS resources and make it a functional test instead of an integ test
IMO there's a tradeoff here, and I'd personally rather see these as functional as opposed to integ tests. This both saves money and improves security for anyone running these tests.
|
|
||
| intent_names = [i.intent.name for i in response.interpretations if i.intent] | ||
| assert "Greeting" in intent_names | ||
| assert "FallbackIntent" in intent_names |
There was a problem hiding this comment.
Is this fallback intent always returned?
| return got_text_response, messages | ||
|
|
||
| async for event in output_stream: | ||
| if isinstance(event, StartConversationResponseEventStreamTextResponseEvent): |
There was a problem hiding this comment.
Why do we not raise a runtime error when there is an unexpected event similar to current bidi integ tests: https://github.com/awslabs/aws-sdk-python/blob/develop/clients/aws-sdk-bedrock-runtime/tests/integration/test_bidirectional_streaming.py#L221-L223
There was a problem hiding this comment.
Your are right to point this out. And I'll add checks for other possible events as well.
SamRemis
left a comment
There was a problem hiding this comment.
Left a few comments - Antonio was very thorough and got most of them.
To make sure I understand - are we expecting this to be the workflow:
- A different PR adds the changelog entry and the models
- We release the client via internal infrastructure
- After confirming that this PR contains only the changes for the integ tests, we merge this PR so these get run for future releases automatically
Is that all correct?
| from aws_sdk_lex_runtime_v2.client import LexRuntimeV2Client | ||
| from aws_sdk_lex_runtime_v2.config import Config | ||
|
|
||
| BOT_ID = os.environ["LEX_BOT_ID"] |
There was a problem hiding this comment.
Thanks for the great write-up here, Antonio :)
While it has it's merits, personally I don't love the idea of having environment variables required for our integ tests at all. If we start adding a bunch of these, they can get out of hand really quickly. It's easy to imagine a scenario where we have
LEX_BOT_ID, but also LEX_BOT_S3_OUTPUT_BUCKET, which is different from TRANSCRIBE_STREAMING_OUTPUT_S3_BUCKET, and just have a mounting list of resources that we all have to make and set in our environment before running the test.
These tests should be self-contained whenever possible. Have we discussed before what our philosophy is for when we should do each of the following?
- Add a new resource via environment variable (as is done in this PR)
- Skip an integ test entirely
- Mock service responses instead of relying on AWS resources and make it a functional test instead of an integ test
IMO there's a tradeoff here, and I'd personally rather see these as functional as opposed to integ tests. This both saves money and improves security for anyone running these tests.
| results = await asyncio.gather(_send_events(stream), _receive_events(stream)) | ||
|
|
||
| got_text_response, messages = results[1] | ||
| assert got_text_response, "Expected to receive a TextResponse event" |
There was a problem hiding this comment.
Nit: I think this assertion is redundant. got_text_response will never be True when len(messages) > 0 is `False.
| Creates a simple Lex V2 bot with a Greeting intent for testing. | ||
|
|
||
| Note: | ||
| This script is intended for local testing only and should not be used for |
There was a problem hiding this comment.
[Discussion]
This setup script approach is understandable (and we already have a similar pattern elsewhere in the repo), but I’d like to clarify the intended long-term workflow for these integration tests:
- What’s the recommended teardown/cleanup path after running the tests (locally and/or in CI)? Should the script also provide a destroy mode?
- For recurring runs (e.g., monthly), do we expect resources to be recreated each time, or reused? If reused, where/how should resource identifiers be stored (config file/SSM/etc.)?
- The doc says “not for production setups” — what environment is expected to run these tests long-term (dedicated test account, ephemeral CI account, etc.)?
If these tests are intended to be temporary, that’s fine — but if they’re long-lived, I think we should document the lifecycle story
| request_config: HTTPRequestConfiguration | None = None, | ||
| ) -> HTTPResponse: | ||
| # Raise the exception with the request object to bypass actual request handling | ||
| raise TestHttpServiceError(request) |
There was a problem hiding this comment.
[Question]
I'm trying to wrap my head around this conceptually - since we have one of these per service, when are we going to move this to a centralized area like smithy-aws-core. That doesn't need to be done in this PR, but all of this duplication is probably something we should start to consolidate
Description of changes:
This PR adds support for the Lex Runtime V2 service by adding the latest Smithy model and code-generating a new client package (
aws-sdk-lex-runtime-v2). It also adds integration tests covering the following patterns:recognize_textstart_conversation(TEXT mode)A setup script at
tests/setup_resources.pycreates the required AWS resources (IAM role, Lex V2 bot, en_US locale with a Greeting intent and closing response).This branch will be rebased once the release automation publishes the client.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.