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.

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

Fix CVE issues #447

Merged
merged 4 commits into from
Mar 19, 2025
Merged

Fix CVE issues #447

merged 4 commits into from
Mar 19, 2025

Conversation

nathaliellenaa
Copy link
Contributor

Description

Fix CVE issues by updating torch and transformers version

Issues Resolved

#438
#439
#441
#442
#443
#444
#445

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@Yerzhaisang
Copy link
Collaborator

Yerzhaisang commented Feb 7, 2025

Hi @nathaliellenaa,

It looks like there’s a CI failure. Could you please rebase your branch onto opensearch-project:main?

Also, could you review the integration test failure logs? We should ensure backward compatibility with previous versions of torch and transformers.

Let me know if you need any help!

Signed-off-by: Nathalie Jonathan <[email protected]>
Signed-off-by: Nathalie Jonathan <[email protected]>
@Yerzhaisang
Copy link
Collaborator

Yerzhaisang commented Feb 12, 2025

I just tried to understand why integration tests fail. Because here previous and latest torch versions generate models with different structures. The model generated by torch 2.0.6 is very hard to deploy and UT fails

Attached the sturcture of these models. Please, take a look
torch_2_0_1.pdf
torch_2_0_6.pdf

@nathaliellenaa
Copy link
Contributor Author

Thanks for your deep dive @Yerzhaisang. I compared the two files and saw that the model in 2.0.1 version uses MultiHeadSelfAttention, while the model in 2.0.6 version uses DistilBertSdpaAttention. I'm not really familiar with these mechanisms, but isn't Scaled Dot-Product Attention less complex and thus easier to deploy compared to Multi-Head Attention?

Note: I've been working to upgrade the PyTorch version in our ml commons repo as well (ref) and I found that we should use PyTorch 2.5.1 version for compatibility with DJL. I created a cluster with the upgraded version and run the UT again and I can see that now it can deploy successfully. However, I'm still encountering connection errors with these changes. So, currently I'm doing a deeper investigation into our codebase to identify what additional modifications might be necessary to resolve this issue.

@Yerzhaisang
Copy link
Collaborator

could you please take a look at this commit ?
I just replaced DistilBertSdpaAttention with MultiHeadSelfAttention. I think it would be good also for backward compatability.

@nathaliellenaa
Copy link
Contributor Author

Accidentally pushed the latest commit.

@nathaliellenaa
Copy link
Contributor Author

@dhrubo-os The changes in this commit fixed the compatibility issues with the upgraded PyTorch version, and all integration tests pass. Is there any concern with replacing DistilBertSdpaAttention with MultiHeadSelfAttention?

@dhrubo-os
Copy link
Collaborator

let me re-run the integ tests again.

@nathaliellenaa
Copy link
Contributor Author

let me re-run the integ tests again.

I haven't include the changes in this commit into this PR. I run the integration tests on my forked repo before (ref)

@dhrubo-os
Copy link
Collaborator

We need to be backward compatible in both places. ML-Commons + Py-ml

I meant we already have pre-trained models: https://opensearch.org/docs/latest/ml-commons-plugin/pretrained-models/

If we upgrade the pytorch, will we be able to run the models which are in the model server already? We need to do more testing on this.

…MultiHeadSelfAttention

Signed-off-by: Nathalie Jonathan <[email protected]>
@nathaliellenaa
Copy link
Contributor Author

I tested all pre-trained models with the upgraded PyTorch version and verified that each model can register, deploy, and predict successfully. I also compared the prediction results for both the old and upgraded PyTorch version, and all results fall within the specified tolerance levels (relative tolerance of 1e-03 and absolute tolerance of 1e-05).

Copy link

codecov bot commented Feb 26, 2025

Codecov Report

Attention: Patch coverage is 92.59259% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.89%. Comparing base (529ee34) to head (7ebd86b).
Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
...search_py_ml/ml_models/sentencetransformermodel.py 92.59% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #447      +/-   ##
==========================================
- Coverage   91.53%   90.89%   -0.65%     
==========================================
  Files          42       43       +1     
  Lines        4395     4656     +261     
==========================================
+ Hits         4023     4232     +209     
- Misses        372      424      +52     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

parent = getattr(parent, part)
return parent, parts[-1]

def patch_model_weights(self, model):
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's add more comments to explain what are we doing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@nathaliellenaa
Copy link
Contributor Author

Hi @Yerzhaisang, since you initiated this commit about the code change in thesentencetransformermodel.py file, can you help review the comments I've added to that file? I want to make sure that the comments are accurate and don't contain any misleading information. Thank you!

Copy link
Collaborator

@Yerzhaisang Yerzhaisang left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! (Please, don't forget to fix the lint)

@dhrubo-os dhrubo-os merged commit fca546c into opensearch-project:main Mar 19, 2025
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants