-
Notifications
You must be signed in to change notification settings - Fork 89
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
Bedrock L2 Construct #688
base: main
Are you sure you want to change the base?
Bedrock L2 Construct #688
Conversation
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.
Overall, the document is well-structured and provides a comprehensive overview of the Bedrock L2 construct. However, enhancing clarity in certain sections and ensuring completeness will greatly benefit users.
|
||
The Bedrock L2 construct simplifies the creation of multiple Bedrock features by providing a wrapper over the Bedrock L1 construct. It exposes functions that enable users to create features with minimal code. Key features include Bedrock Agent, Knowledge Base, Guardrails, Inference Profiles, and Prompt. | ||
|
||
|
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.
Please ensure that all sections of the new construct are complete and provide sufficient details for users to understand and implement the construct.
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 @hrudu-dev , yes we have added all the construct props , API documentation and initializer code for all the resources.
text/0686-bedrock-l2-construct.md
Outdated
For more details please refer here [Amazon Bedrock README](https://github.com/awslabs/generative-ai-cdk-constructs/blob/main/src/cdk-lib/bedrock/README.md). | ||
|
||
|
||
## Knowledge Base |
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.
Verify that the documentation (README) is clear and includes examples for each feature of the Bedrock L2 construct. It should also explain how to integrate with other AWS services.Verify that the documentation (README) is clear and includes examples for each feature of the Bedrock L2 construct. It should also explain how to integrate with other AWS services.
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.
Yes, the readme has information on how to create , KB with different vector stores and data sources with different strategies. same for agent , prompt and inference profiles. for details example we have another repo under aws-samples org which use these construct and create a full fledge solution.
text/0686-bedrock-l2-construct.md
Outdated
const auroraDb = aurora.AmazonAuroraVectorStore.fromExistingAuroraVectorStore(stack, 'ExistingAuroraVectorStore', { | ||
clusterIdentifier: 'aurora-serverless-vector-cluster', | ||
databaseName: 'bedrock_vector_db', | ||
schemaName: 'bedrock_integration', |
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.
Confirm that all chunking strategies are thoroughly explained and example code is provided for each (Fixed Size, Hierarchical, Semantic, etc.
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.
yes. done.
|
||
const kb = new KnowledgeBase(stack, 'MyKnowledgeBase', { | ||
name: 'MyKnowledgeBase', | ||
embeddingsModel: BedrockFoundationModel.COHERE_EMBED_MULTILINGUAL_V3, |
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.
Review the guardrails to ensure that they cover all necessary areas for content filtering, denied topics, word filtering, and sensitive information filters.
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.
yes. done
|
||
- **Fixed Size Chunking**: This method divides the data into fixed-size chunks, with each chunk | ||
containing a predetermined number of tokens. This strategy is useful when the data is uniform | ||
in size and structure. |
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.
Suggest adhering to AWS best practices, especially in areas involving security (e.g., handling AWS Secrets Manager, IAM roles).
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.
yes, done.
|
||
const variant2 = PromptVariant.text({ | ||
variantName: "variant2", | ||
model: claudeModel, |
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.
Consider adding a section on how users can provide feedback or report issues with the Bedrock L2 construct.
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.
once the construct is pushed to aws-cdk repo, one can open an issue for the construct in that repo.
|
||
## Prompt management | ||
|
||
Amazon Bedrock provides the ability to create and save prompts using Prompt management so that you can save |
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.
Clarify the versioning strategy for the constructs. It’s important for users to know how to manage updates and changes.
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.
Example of PromptVariant
describe the same.
text/0686-bedrock-l2-construct.md
Outdated
blockedOutputsMessaging: 'blockedOutputsMessaging', | ||
name: 'namemycfnguardrails', | ||
wordPolicyConfig: { | ||
wordsConfig: [ |
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.
Please include a section on testing the Bedrock L2 construct to ensure users understand how to validate their implementations.
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.
The code snippet added shows how to use the construct. The final code will have unit test and integ test to validate the implementation.
|
||
#### Specific version | ||
|
||
You can use the `AgentAlias` resource if you want to create an Alias for an existing Agent. |
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.
It would be beneficial to include error handling examples in the documentation to guide users on how to manage potential issues.
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.
The construct error handling return appropriate user message at the time of cdk synth.
### Create an Agent | ||
|
||
The following example creates an Agent with a simple instruction and default prompts that consults a Knowledge Base. | ||
|
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.
Ensure that all example code snippets are well-commented and easy to follow. Consider adding more context or explanations for complex sections.
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.
noted. with code checkin we can review that again.
I've reviewed the documentation and it looks great! Happy to contribute further if needed. Thank you! |
|
||
### What are the drawbacks of this solution? | ||
|
||
The Knowledge Base vector stores (OpenSearch and Aurora clusters) utilize custom resource lambda functions, as there are no underlying L1 constructs available. |
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.
I see there's CFN resource for OpenSearch collection with vector search. https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-opensearchserverless-collection.html
Is it different that what need 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.
Yes, we have CFN resource for vector collection but not for vector index creation. The custom resource used here create a vector index. But like we discussed we can skip the vector index part from the construct and enable it to use an existing collection and index.
text/0686-bedrock-l2-construct.md
Outdated
const importedGuardrail = bedrock.Guardrail.fromCfnGuardrail(cfnGuardrail); | ||
``` | ||
|
||
Python |
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 can remove the Python code here in readme and RFC.
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.
Done, will push with next commit.
text/0686-bedrock-l2-construct.md
Outdated
|
||
### Why should we _not_ do this? | ||
|
||
The construct is published via the [generative-ai-cdk-constructs](https://github.com/awslabs/generative-ai-cdk-constructs/blob/main/src/cdk-lib/bedrock/README.md) repository. However, due to the increasing demands and expanding use cases, maintaining it within this repository has become challenging. |
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.
maintaining it within this repository has become challenging.
Why maintaining it within this repo is more challenging than merging into aws-cdk
?
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.
aws-gen-ai-cdk-construct was an experimental initiative with a small team. We feel aws-cdk is the right place for this construct on long term.
``` | ||
|
||
|
||
#### Knowledge Base - Data Sources |
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.
Do these APIs just create AWS::Bedrock::DataSource
CFN resources?
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.
Yes
text/0686-bedrock-l2-construct.md
Outdated
* **API Bar Raiser**: @{BAR_RAISER_USER} | ||
|
||
|
||
The Bedrock L2 construct simplifies the creation of multiple Bedrock features by providing a wrapper over the Bedrock L1 construct. It exposes functions that enable users to create features with minimal code. Key features include Bedrock Agent, Knowledge Base, Guardrails, Inference Profiles, and Prompt. |
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.
How does it simplify the creation of Bedrock features? Could you provide a comparison between using L1 and L2?
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.
text/0686-bedrock-l2-construct.md
Outdated
* **API Bar Raiser**: @{BAR_RAISER_USER} | ||
|
||
|
||
The Bedrock L2 construct simplifies the creation of multiple Bedrock features by providing a wrapper over the Bedrock L1 construct. It exposes functions that enable users to create features with minimal code. Key features include Bedrock Agent, Knowledge Base, Guardrails, Inference Profiles, and Prompt. |
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.
Knowledge Base
is not in the scope of this RFC and L2 at the moment
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.
Apologies for any confusion but Knowledge base can still be the part of the bedrock L2 construct . But the vector store based KB will not be part of the scope since it needs a custom resource to create an index. For this I have updated the documentation create vector store knowledge . Basically, the construct will expect an existing vector store . It supports opensearch serverless, amazon aurora and pinecone as existing vector store. This approach will have no custom resource in bedrock L2 construct.
text/0686-bedrock-l2-construct.md
Outdated
|
||
The resource accepts an `instruction` prop that is provided to any Bedrock Agent it is associated with so the agent can decide when to query the Knowledge Base. | ||
|
||
Amazon Bedrock Knowledge Bases currently only supports S3 as a data source. The `S3DataSource` resource is used to configure how the Knowledge Base handles the data source. |
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.
only supports S3 as a data source.
In the sectionData Source
, it mentions there are multiple data source supported. What's the difference?
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.
Typo error. removed it in 93e54fd
text/0686-bedrock-l2-construct.md
Outdated
|
||
```ts | ||
import * as s3 from 'aws-cdk-lib/aws-s3'; | ||
import { bedrock } from '@cdklabs/generative-ai-cdk-constructs'; |
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.
Mention why we need to use an external construct.
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 a typo. Replaced it with import { bedrock } from 'aws-cdk-lib/aws-bedrock';. Eventually when the construct will be pushed to aws-cdk-lib it can be imported in this fashion. 93e54fd
text/0686-bedrock-l2-construct.md
Outdated
|
||
Amazon Bedrock Knowledge Bases enable you to provide foundation models and agents with contextual information from your company’s private data sources. This enhances the relevance, accuracy, and customization of their responses. | ||
|
||
### Create a Knowledge Base |
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.
Since this is currently out of scope for Bedrock L2 in CDK, we can make this section short and refer to the readme in the other repo
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.
Apologies for any confusion but Knowledge base can still be the part of the bedrock L2 construct . But the vector store based KB will not be part of the scope since it needs a custom resource to create an index. For this I have updated the documentation create vector store knowledge . Basically, the construct will expect an existing vector store . It supports opensearch serverless, amazon aurora and pinecone as existing vector store. This approach will have no custom resource in bedrock L2 construct.
- **Amazon S3**. You can either create a new data source using the `bedrock.S3DataSource(..)` class, or using the | ||
`kb.addS3DataSource(..)`. |
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.
Which is the recommended way? IMO kb.addS3DataSource()
is a better API. If data source is not a stand alone resource, we can just keep the add method.
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.
It depends. If someone want to use their existing kb and want to associate a datasource using construct, they can use bedrock.S3Datasource. If they have an existing datasource and want to use kb using construct they can use kb.addS3DataSource. we have seen demand for both scenarios.
text/0686-bedrock-l2-construct.md
Outdated
Content filters – Adjust filter strengths to block input prompts or model responses containing harmful content. | ||
|
||
Denied topics – Define a set of topics that are undesirable in the context of your application. These topics will be blocked if detected in user queries or model responses. | ||
|
||
Word filters – Configure filters to block undesirable words, phrases, and profanity. Such words can include offensive terms, competitor names etc. | ||
|
||
Sensitive information filters – Block or mask sensitive information such as personally identifiable information (PII) or custom regex in user inputs and model responses. |
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.
Let's add some bullet points here so it's easier to read.
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.
Done - 93e54fd
text/0686-bedrock-l2-construct.md
Outdated
const app = new cdk.App(); | ||
const stack = new cdk.Stack(app, 'aws-cdk-bedrock-data-sources-integ-test'); |
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.
These should be removed. Also we should keep the code example as minimal as possible. Ideally it should only includes required property unless that property is for a specific feature.
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.
Done - 01e9249
text/0686-bedrock-l2-construct.md
Outdated
Amazon Bedrock Knowledge Bases provides foundation models and agents with contextual data from private sources. This capability enhances | ||
response relevance, accuracy, and customization for your specific use cases. | ||
|
||
## Supported Knowledge Base Types |
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.
nit: this should be ###
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.
Done.
text/0686-bedrock-l2-construct.md
Outdated
|
||
2. `instruction` prop: Provided to associated Bedrock Agents to determine when to query the Knowledge Base | ||
|
||
3. embeddingsModel: Foundation model supported with bedrock. |
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.
3. embeddingsModel: Foundation model supported with bedrock. | |
3. `embeddingsModel` prop: Foundation model supported with bedrock. |
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.
Accepted
|
||
To create a vector knowledge Base, a vector index on a vector store is required. The resource accepts: | ||
|
||
1. `storageConfiguration` prop: An existing vector store 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.
Looking at https://docs.aws.amazon.com/bedrock/latest/APIReference/API_agent_StorageConfiguration.html, there are 2 more configuration supported: mongoDbAtlasConfiguration and redisEnterpriseCloudConfiguration. Are they supported?
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.
No , not yet, but may be in Future.
embeddingsModel: bedrock.BedrockFoundationModel.TITAN_EMBED_TEXT_V1, | ||
instruction: 'Use this knowledge base to answer questions about books. ' + 'It contains the full text of novels.', | ||
storageConfiguration:[{ | ||
type:'OPENSEARCH_SERVERLESS', |
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.
Later in the implementation, it should be a static variable.
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.
Agreed. Noted.
text/0686-bedrock-l2-construct.md
Outdated
N | ||
ote - you need to provide clusterIdentifier, databaseName, vpc, secret and auroraSecurityGroupId used in |
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.
nit: extra line 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.
Ack.
text/0686-bedrock-l2-construct.md
Outdated
const docBucket = new s3.Bucket(this, "DocBucket"); | ||
|
||
new bedrock.S3DataSource(this, "DataSource", { | ||
bucket: docBucket, | ||
knowledgeBase: kb, | ||
dataSourceName: "books", | ||
chunkingStrategy: bedrock.ChunkingStrategy.FIXED_SIZE, | ||
}); | ||
``` |
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.
Same 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.
Done
text/0686-bedrock-l2-construct.md
Outdated
import * as s3 from 'aws-cdk-lib/aws-s3'; | ||
import { pinecone, bedrock } from 'aws-cdk-lib/aws-bedrock'; | ||
|
||
const pineconeds = new pinecone.PineconeVectorStore({ |
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.
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.
Removed. same as AmazonAuroraVectorStore it can go as separate construct and then add a from method in bedrock construct.
text/0686-bedrock-l2-construct.md
Outdated
const docBucket = new s3.Bucket(this, 'DocBucket'); | ||
|
||
new bedrock.S3DataSource(this, 'DataSource', { | ||
bucket: docBucket, | ||
knowledgeBase: kb, | ||
dataSourceName: 'books', | ||
chunkingStrategy: bedrock.ChunkingStrategy.FIXED_SIZE, | ||
}); | ||
``` |
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.
Same. We don't need data source code 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.
Done
kb.addWebCrawlerDataSource({ | ||
sourceUrls: ['https://docs.aws.amazon.com/'], | ||
chunkingStrategy: ChunkingStrategy.HIERARCHICAL_COHERE, | ||
customTransformation: CustomTransformation.lambda({ | ||
lambdaFunction: lambdaFunction, | ||
s3BucketUri: `s3://${bucket.bucketName}/chunk-processor/`, | ||
}), | ||
}); |
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.
Instead of putting examples all together. Can we put them individual section above? It would be easier to read.
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.
Done.
text/0686-bedrock-l2-construct.md
Outdated
```ts | ||
CustomTransformation.lambda({ | ||
lambdaFunction: lambdaFunction, | ||
s3BucketUri: `s3://${bucket.bucketName}/chunk-processor/`, | ||
}), | ||
``` |
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.
nit: format
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.
Done
text/0686-bedrock-l2-construct.md
Outdated
|
||
### Prepare the Agent | ||
|
||
he Agent constructs take an optional parameter shouldPrepareAgent to indicate that the Agent should be prepared after any updates to |
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.
he
typo?
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.
Fixed.
const agentAlias2 = new bedrock.AgentAlias(this, 'myalias2', { | ||
aliasName: 'myalias', | ||
agent: agent, | ||
agentVersion: '1', // optional | ||
description: 'mytest' | ||
}); |
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 seems to be 1:1 mapping to L1 resource. IMO we can replace it with an API agent.addAlias(...)
. What do you think?
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 started with addAlias
method initially but then we faced some issue and then had to switch to this way of creation. So instead of agent adding alias the control is with alias to associate an agent to it. This class apart from initializing agent also support method like fromAttributes
. This method can import and existing alias created outside of CDK. Added it in new commit.
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.
What's the issue with addAlias
method? AgentAlias
is essentially the same as L1 and if we need it, we can just use L1. For the import, I think it's a very edge case where agent is created in the stack while importing an alias created somewhere else.
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.
The fromAttributes
is a must to handle the default test alias of bedrock agent (created by the service itself).
Can't recall the issues with addAlias
method , I think we can bring it back and verify there are no issue because of it.
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.
sounds good.
This is a request for Amazon Bedrock L2 construct that simplifies the creation of multiple Bedrock features by providing a wrapper over the Bedrock L1 construct. It exposes functions that enable users to create features with minimal code. Key features include Bedrock Agent, Knowledge Base, Guardrails, Inference Profiles, and Prompt. See #686 for
additional details.
APIs are signed off by @{BAR_RAISER}.
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license