Smithy BiDi streaming support + Bedrock Migration #3675
Conversation
sbaluja
changed the title
|
Test |
| Aws::BedrockRuntime::BedrockRuntimeClientConfiguration()); | ||
|
|
||
| /* Legacy constructors due deprecation */ | ||
| /** |
There was a problem hiding this comment.
do we want to add a "legacy" constructor? if we're adding one constructor i would say just add the first one, and if someone wants to provide a bearer auth provider, they have to use the newer version of constructor.
| /** | ||
| * Initializes client to use BearerTokenAuthSignerProvider, with default http client factory, and optional client config. | ||
| */ | ||
| BedrockRuntimeClient(const Aws::Auth::BearerTokenAuthSignerProvider& bearerTokenProvider, |
There was a problem hiding this comment.
just thinking out loud here for a second, this constructor allows someone to use the BearerTokenAuthSignerProvider in the bedrock client. IF the operation allows for it, it will use bearer auth, otherwise it will fail?
Im trying to think of a world where a customer whats to mix sigv4 and bearer auth in the same client and how those could live together. is it possible to create a constructor that take a list of "signer providers" somehow and resolves at runtime?
| Aws::MakeShared<smithy::GenericAuthSchemeResolver<>>( | ||
| ALLOCATION_TAG, Aws::Vector<smithy::AuthSchemeOption>({smithy::SigV4AuthSchemeOption::sigV4AuthSchemeOption, | ||
| smithy::BearerTokenAuthSchemeOption::bearerTokenAuthSchemeOption})), | ||
| { |
There was a problem hiding this comment.
OK this is sort of what i was alluding to earlier should we just allow a user to override the whole map and say "bring whatever you want", so that we arent bearer token specific, just, if you want to override this mapping, bring your own mapping.
| auto requestCopy = Aws::MakeShared<InvokeModelWithBidirectionalStreamRequest>("InvokeModelWithBidirectionalStream", request); | ||
| requestCopy->SetBody(eventEncoderStream); // this becomes the body of the request | ||
| request.SetBody(eventEncoderStream); | ||
| request.SetBody(eventEncoderStream); // this becomes the body of the request |
| * Smithy-compatible bi-directional streaming task for modern AWS clients | ||
| */ | ||
| template <typename OutcomeT, typename ClientT, typename RequestT, typename HandlerT> | ||
| class AWS_CORE_LOCAL SmithyBidirectionalStreamingTask final { |
There was a problem hiding this comment.
maybe we want to put this in the smithy namespace and not in core as this is the sra client specific
| : m_client(client), m_request(request), m_handler(handler), m_context(context), m_stream(stream), | ||
| m_sem(Aws::MakeShared<Aws::Utils::Threading::Semaphore>("SmithyBidirectionalStreamingTask", 0, 1)) { | ||
|
|
||
| m_authCallback = std::move(authCallback); |
There was a problem hiding this comment.
moving this call back scares me a little bit about lifetimes, consider the following greatly simplified example
#include <future>
#include <iostream>
class bedrock_runtime_client {
public:
std::future<void> invoke_bidi_async() {
std::string endpoint{"https://aws.amazon.com"};
auto value_callback = [&endpoint]() -> void {
std::this_thread::sleep_for(std::chrono::seconds(1));
std::cout << endpoint << std::endl;
};
return std::async(std::launch::async, std::move(value_callback));
}
};
auto main() -> int {
bedrock_runtime_client client{};
auto run_future = client.invoke_bidi_async();
run_future.wait();
return 0;
}Since we construct the callable async task with local variables does this cause lifetime issues? i could have simplified this wrong, but it seems like there might be lifetime issues. looking at where you call this it seems like you pass by value which will copy the relevant parameters.
At the end of the day, is it possible that we run into lifetime issues, and if so, we cant do that
There was a problem hiding this comment.
I think this is actually fine. Since std::move is passing ownership, this isn't much different from what we currently do with the endpoint callbacks in MakeRequestDeserialize, with the difference being ownership transferred to an intermediary class (SmithyBidirectionalStreamingTask), before making it's way to an rvalue assignment to MakeRequestDeserialize. I think the bigger concern is the fact that the authCallback captures a local variable eventEncoderStream since that reference is pointing to the original scope. BUT this is also fine because eventEncoderStream is a shared pointer, and the pass by copy will increment the use count, and it also lives throughout the lifetime of the request (SetBody holds a reference to this too).
|
|
||
| message.InsertEventHeader(EVENTSTREAM_DATE_HEADER, EventHeaderValue(now.Millis(), EventHeaderValue::EventHeaderType::TIMESTAMP)); | ||
| message.InsertEventHeader(EVENTSTREAM_SIGNATURE_HEADER, std::move(finalSignatureDigest)); | ||
| for (auto&& header : message.GetEventHeaders()) { |
There was a problem hiding this comment.
nit use std::for_each for a nice one liner
3 participants
Issue #, if available:
Description of changes:
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.