-
Notifications
You must be signed in to change notification settings - Fork 199
feat: Add document creation for ChatBedrockConverse #601
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
base: main
Are you sure you want to change the base?
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.
Thank you for your contribution @hatMatch !
" must have a dictionary with a valid s3 uri as a dict." | ||
) | ||
|
||
if source.get("content") and not isinstance(source.get("content", list)): |
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.
Passing content
source currently fails because isinstance
is missing the second argument for type 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.
whoops good catch thanks!
Args: | ||
name: The name of the document. | ||
source: The source of the document. | ||
context: Info for the model to understand the document for citations. | ||
format: The format of the document, or its extension. |
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.
Add an enable_citations
docstring for completeness
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.
will do!
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 in 4812a05
format: The format of the document, or its extension. | ||
Returns: | ||
Dictionary containing a properly formatted to add to message content.""" | ||
if re.match(r"[^\w\[\]\(\)-]|[\s]{2,}", name): |
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.
re.match
won't work here for all cases here, as this will only matches for specific invalid chars/sequences starting from the first character (for example, No Cite
won't be caught).
You should use re.search
instead (+ simplify a bit):
if re.match(r"[^\w\[\]\(\)-]|[\s]{2,}", name): | |
if not re.search(r"[^A-Za-z0-9 \[\]()\-]|\s{2,}", name): |
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 think not was accidentally added to the front. Resolved in d757851
Co-authored-by: Michael Chin <[email protected]>
Looking to follow up with this sometime next week. Will be out for a few days. Cheers and thanks for looking through this! |
…ters not found using re.search instead of found
# Skip creating new client if passed in constructor | ||
if self.client is None: | ||
self.client = create_aws_client( |
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 this was removed unintentionally in the 40241bd merge, let's put it back to fix the tests
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.
added it back in but I did change my setup so hoping nothing else broke.
Add document creation for ChatBedrockConverse
Implements document creation to add to message content. Does moderate error handling on input types though a pydantic model for the various parameters or document itself might be preferable.
Usage