-
Notifications
You must be signed in to change notification settings - Fork 188
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
Upgrade Pillow and nltk for vulnerability reasons #989
Conversation
f6c901c
to
2d086a1
Compare
4d29cc3
to
8142bbc
Compare
@@ -30,6 +30,12 @@ def _splitting_functions(split_by: str, language: str='english') -> FunctionType | |||
except LookupError: | |||
nltk.download("punkt") | |||
|
|||
# Punkt_tab needs to be downloaded after NLTK 3.8 and later |
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 is this doing?
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 is needed in the text split. Previously it was downloaded along with "punkt"
but now we need to download it explicitly after the upgrade.
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.
An error will be raised if we do not do this downloading
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
bug fix
What is the current behavior? (You can also link to an open issue here)
The existing pillow and nltk dependencies have vulnerability concerns
What is the new behavior (if this is a feature change)?
we upgrade pillow to 10.4.0 and nltk to 3.9.1
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
no
Have unit tests been run against this PR? (Has there also been any additional testing?)
no
Related Python client changes (link commit/PR here)
no
Related documentation changes (link commit/PR here)
Manual tests have been done to ensure that these changes won't change the embeddings.
Other information:
no
Please check if the PR fulfills these requirements