Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix SQLAlchemy truncating text when it is too big #5206

Merged
merged 6 commits into from Jun 2, 2023
Merged

Fix SQLAlchemy truncating text when it is too big #5206

merged 6 commits into from Jun 2, 2023

Conversation

wsantos
Copy link
Contributor

@wsantos wsantos commented May 24, 2023 •
edited

Fixes SQLAlchemy truncating the result if you have a big/text column with many chars.

SQLAlchemy truncates columns if you try to convert a Row or Sequence to a string directly

For comparison:

  • Before:
    [('Harrison', 'That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio ... (2 characters truncated) ... hat is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio ')]

  • After:
    [('Harrison', 'That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio ')]

Who can review?

Community members can review the PR once tests pass. Tag maintainers/contributors who might be interested:

I'm not sure who to tag for chains, maybe @vowelparrot ?

@wsantos wsantos changed the title Fix SQLAlchemy truncating text when is to big Fix SQLAlchemy truncating text when it is too big May 24, 2023
hwchase17
hwchase17 reviewed May 25, 2023
View reviewed changes
Copy link
Contributor

@hwchase17 hwchase17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so on one hand this seems reasonable

on the other hand, this may actually weirdly be desirable as LLMs have a limited context length so tough to make work with long strings so this could have actually been saving some stuff...

@wsantos
Copy link
Contributor Author

wsantos commented May 25, 2023

so on one hand this seems reasonable

on the other hand, this may actually weirdly be desirable as LLMs have a limited context length so tough to make work with long strings so this could have actually been saving some stuff...

Sure it could be saving usage, but for me, it's sometimes hallucinating and sometimes it returns the action JSON, like here:

image

Maybe it's because the truncation is happening in the middle of the text and cutting worlds, for my scenario I do need those values I know they are big but it's a requirement I have, do you think we should add an option like max_char and that is applied on every column and the truncate happens at the end, without truncating word but adding ...? so it's going to be a conscient decision and not a hidden one?

@wsantos
Copy link
Contributor Author

wsantos commented May 25, 2023

@hwchase17 I'm going to add this truncate feature and make it default to the same as in SQLAchemy so we keep the behavior but the user can decide if they want to change it or not, I've one question is there a good place to put utility functions, I'd like to put that string truncate function in a place that can be reused, utils.py perhaps?

@wsantos
Copy link
Contributor Author

wsantos commented May 26, 2023

@hwchase17 I've added options to change the "truncation" size and add defaults so the behavior will be almost the same as before, but cleaner and without truncating the chain, let me know if you want any other changes.

@wsantos
added options to change the column limit when converting to string an…
de89d59 
…d added default to keep the same behavior as before.
eyurtsev
eyurtsev reviewed May 26, 2023
View reviewed changes
langchain/utils.py Outdated Show resolved Hide resolved
langchain/sql_database.py Outdated Show resolved Hide resolved
langchain/utils.py Outdated Show resolved Hide resolved
eyurtsev
eyurtsev reviewed May 26, 2023
View reviewed changes
langchain/sql_database.py Outdated Show resolved Hide resolved
tests/unit_tests/test_sql_database.py Outdated Show resolved Hide resolved
langchain/utils.py Outdated Show resolved Hide resolved
@wsantos
Copy link
Contributor Author

wsantos commented May 28, 2023

@eyurtsev I think I've covered it all, let me know if we need more changes

@wsantos
Copy link
Contributor Author

wsantos commented Jun 1, 2023

Hey @eyurtsev do you wanna any other fix in this PR? thanks in advance.

@eyurtsev eyurtsev added lgtm PR looks good. Use to confirm that a PR is ready for merging. maintainer-to-merge labels Jun 2, 2023
@eyurtsev
Copy link
Collaborator

eyurtsev commented Jun 2, 2023

Thanks for pinging me! @wsantos

@eyurtsev eyurtsev merged commit db45970 into langchain-ai:master Jun 2, 2023
12 checks passed
@danielchalef danielchalef mentioned this pull request Jun 5, 2023
Merged
Undertone0809 pushed a commit to Undertone0809/langchain that referenced this pull request Jun 19, 2023
@wsantos @Undertone0809
Fix SQLAlchemy truncating text when it is too big (langchain-ai#5206)
73f6953 
# Fixes SQLAlchemy truncating the result if you have a big/text column
with many chars.

SQLAlchemy truncates columns if you try to convert a Row or Sequence to
a string directly

For comparison:

- Before:
```[('Harrison', 'That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio ... (2 characters truncated) ... hat is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio That is my Bio ')]```

- After:
```[('Harrison', 'That is my Bio That is my Bio That is my Bio That is
my Bio That is my Bio That is my Bio That is my Bio That is my Bio That
is my Bio That is my Bio That is my Bio That is my Bio That is my Bio
That is my Bio That is my Bio That is my Bio That is my Bio That is my
Bio That is my Bio That is my Bio ')]```



## Who can review?

Community members can review the PR once tests pass. Tag
maintainers/contributors who might be interested:

I'm not sure who to tag for chains, maybe @vowelparrot ?
This was referenced Jun 25, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eyurtsev eyurtsev eyurtsev left review comments

@hwchase17 hwchase17 hwchase17 left review comments

Assignees
No one assigned
Labels
lgtm PR looks good. Use to confirm that a PR is ready for merging.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@wsantos @eyurtsev @hwchase17