fix: fix issue where cdk bootstrap failed and add response streaming feature (toggle off). Close #122 #134
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* fix: vscode settings update (use v1.85+) * fix(projenrc): configUpgradeDeps at postSynth to conform with pdk upgrade * fix(api): use custom python dep to conform with pdk upgrade * fix(galileo-cdk): bump jsii version and add pnpm-logger dep to conform pdk upgrade * chore(deps): upgrade pdk@0.23.31, cdk@2.137.0 and fix projen dep * chore(deps): remove sdk/smithy version pinning, replace discont'd aws-sdk packages * fix(website): build fails in CI * chore(ws-api): add ws-api bootstrap * fix(cli): non-built project bootstrap fails fixes aws-samples#112 * fix: adjust tsconfig for api and website * fix(infra): admin user attachment depends on already created group and user * feat(ws-api): rename to ws-api, add model and wire up with infra * chore(deps): bump pdk to 0.23.33 and align tsconfigs for api and web * feat(infra): wire up wsApi with lambdaAuthorizer and bump node runtime (20_x) for lambda handlers * fix(website): popover dismissbutton and header must be present together * feat(galileo-sdk): add streaming support to chain * feat: dependency management and project wiring * feat(infra): add ws connections table * feat(infra): split inference engine handler rest/ws with shared logic * feat(infra): merge inference and presentation stacks to avoid circular deps * feat(ws-api): connection data persistence * feat(website): switch to streaming respose in chat * fix(api): poetry lockfile causes CI build error * fix(ws-api): don't generate code for send-chat-message
jessieweiyi
changed the title
martenpayne
requested changes
May 27, 2024
demo/infra/src/application/ai/inference-engine/engine/handler/ws-handler/index.ts
Outdated
Show resolved
Hide resolved
chn217
reviewed
May 27, 2024
chn217
reviewed
May 27, 2024
There was a problem hiding this comment.
Nice work on this PR! 👏
Just a general comment: I noticed that there are a few TODOs included in this PR. It would be good to track these TODOs in separate issues in the backlog to ensure they don't get lost or forgotten.
The most important missing TODO seems to be related to documentation (DOC). We should create a separate issue for this and prioritize it accordingly.
martenpayne
approved these changes
May 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR includes @sperka PR #130. The PR has been tested on different use cases and fix a few issues:
The PR also:
Issues found in the streaming feature, so add a feature flag on the feature to toggle it off:
To toggle on: add ?features=streaming query string to the url
Pending To-do items:
lambda-powertoolsto v2Here is a copied of original PR description:
This PR introduces streaming support and was also targeting a bit of a cleanup/refresh.
latestorlatest-that-doesn't-break-anythingtype-safe-apicreate-messagehandler to supportRESTandWSThere are a few things that should be added or changed (refer to
// TODOs in the code):langchain@0.0.194version is used, there were huge improvements so updating to latest should be inevitable - however, this includes quite a bit of effortlambda-powertoolsv2 is out, but has breaking changesqaChaindefinition and how streaming is handled, especially based on the models used. With the current generic approach, streaming is not happening, there is only one chunk returned (which is the actual result) instead of incremental answer (aka tokens). This may be an issue because of the langchain version, or something else. That part needs to be tested, both for bedrock LLMs and Sagemaker-hosted LLMs.operationvalue can be any string value which should be restricted toenum, and maybe other fields could be limited as well -- needs thorough reviewDoes this PR introduce a breaking change?
No breaking change. The streaming feature is under a feature toggle.
Related Issues/Discussion
How Has This Been Tested?
Screenshots (if appropriate)
PR Checklist
IMPORTANT: Please review the CONTRIBUTING.md file for detailed contributing guidelines.