-
Notifications
You must be signed in to change notification settings - Fork 24
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
Added new seadfarmer module for Question and Answering Gen AI construct #81
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
) | ||
|
||
# get an existing userpool | ||
cognito_pool_id = cognito_pool_id |
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.
Is Congnito mandatory? Can we add a parameter to support both IAM and Cognito?
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.
As of today cognito is mandatory to be used with this construct. Created a feature request on the construct to make cognito user pool id as optional parameter
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.
Thank you!
manifests/qna-rag-modules.yaml
Outdated
path: modules/fmops/qna-rag | ||
parameters: | ||
- name: cognito-pool-id | ||
value: us-east-1-XXXXX |
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.
Looks like we need an IDF module for Cognito. Can you move the manifest in examples/manifests/
since this requires user input/pre-setup Conginto user pool? We can move it back once we have congnito module/or IAM-only auth mode.
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.
Moved the manifest to examples
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.
Thanks!
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
vpc_id = os.getenv(_param("VPC_ID")) | ||
cognito_pool_id = os.getenv(_param("COGNITO_POOL_ID")) | ||
os_domain_endpoint = os.getenv(_param("OS_DOMAIN_ENDPOINT")) | ||
os_security_group_id = os.getenv(_param("OS_SECURITY_GROUP_ID")) |
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.
Minor query - can you please add ability to provide input asset bucket & enable observability (ref params here)?
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.
Looks great, thank you!
I added a few minor comments, also as discussed this covers the consumer part, and in the next PRs would be great to see also the ingestion/embeddings generation for a complete E2E flow. Thanks!
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
…cket as optional input to RAG module
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Looks good, thank you!
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
manifests/storage-modules.yaml
Outdated
@@ -51,3 +51,25 @@ parameters: | |||
value: 30 | |||
- name: removal-policy | |||
value: DESTROY | |||
--- | |||
name: opensearch | |||
path: git::https://github.com/awslabs/idf-modules.git//modules/storage/opensearch/ |
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 should lock this version to a release of IDF:
git::https://github.com/awslabs/idf-modules.git//modules/storage/opensearch?ref=release/1.7.0&depth=1
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.
updated the manifest
@@ -0,0 +1,22 @@ | |||
--- | |||
name: opensearch | |||
path: git::https://github.com/awslabs/idf-modules.git//modules/storage/opensearch/ |
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 should lock this version to a release of IDF:
git::https://github.com/awslabs/idf-modules.git//modules/storage/opensearch?ref=release/1.7.0&depth=1
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.
Updated the manifest
modules/fmops/qna-rag/stack.py
Outdated
) | ||
os_security_group.add_ingress_rule( | ||
peer=security_group, | ||
connection=ec2.Port.tcp(443), |
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.
Where is this port number coming from?
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.
This is the port on which open search is listening, Thus allowing traffic to Open search from lambda on port 443. Can make this configurable if that makes sense.
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.
Made the port configurable parameter with default value as 443
…dded Port as configurable parameter
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Describe your changes
Added new seadfarmer module for Question and Answering Gen AI construct
Issue ticket number and link
Checklist before requesting a review
CHANGELOG.MD
with a description of my changesscripts/validate.sh
)seedfarmer apply
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.