Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add cross encoder support #1615
Add cross encoder support #1615
Changes from all commits
3e9526b
a840ae7
3734ac4
2ffea39
a03e7ce
15e81bc
8dce6a9
7871d8c
26bcbcd
c32cf9e
0fc9028
4ea18fa
9d4e083
6d7b429
97e10b1
09e0f91
b3e90b5
be113ed
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 49 in common/src/main/java/org/opensearch/ml/common/FunctionName.java
Codecov / codecov/patch
common/src/main/java/org/opensearch/ml/common/FunctionName.java#L49
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.
@ylwu-amzn did we wrap up this discussion about adding
Aryn
in the code 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.
as I understand, all this does is protect us on the off-chance that AWS does what Elastic did a few years ago and switches out the license or something. This is coming from Mehul, who oversaw that whole transition (elastic fork -> opensearch).
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 need to worry that actually, if you check Elastic, community can still use the open source version even they changed license, otherwise we can't do "elastic fork -> opensearch". If you worry this part, I think we should suggest not adding Aryn to license header. What if some other company make a change to this file, they may also prefer to add their license. Then in future how can Aryn tell which company holds the license for this file? It looks not reasonable that not allowing other guys modifying your code, right?
I guess the other reason for adding Aryn is to show the credit of this feature. We are planning to build some way to show attribution/appreciation for features from community contributors. For example, we can maintain a contribution list file like this
Any suggestion?
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 already decided this with @sean-zheng-amazon and @mashah
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.
@HenryL27 , Can you please point to the discussions where this is decided and what exactly is the decision?
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 was decided prior to the 2.10 release as part of a PR that we contributed. I believe the initial discussion was done in that PR, but was later concluded over a call.
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
Check warning on line 257 in common/src/main/java/org/opensearch/ml/common/input/MLInput.java
Codecov / codecov/patch
common/src/main/java/org/opensearch/ml/common/input/MLInput.java#L256-L257
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 we have test for this section?
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.
@dhrubo-os jacoco is showing red for like, this entire section (despite my attempts to the contrary). I think what's happening is that the classLoader stuff at the top of this method preempts all the MLInput parsing logic and relegates it to the subclass. We should probly just remove this stuff, idk?
Check warning on line 269 in common/src/main/java/org/opensearch/ml/common/input/MLInput.java
Codecov / codecov/patch
common/src/main/java/org/opensearch/ml/common/input/MLInput.java#L269
Check warning on line 62 in common/src/main/java/org/opensearch/ml/common/input/nlp/TextSimilarityMLInput.java
Codecov / codecov/patch
common/src/main/java/org/opensearch/ml/common/input/nlp/TextSimilarityMLInput.java#L62
Check warning on line 111 in common/src/main/java/org/opensearch/ml/common/input/nlp/TextSimilarityMLInput.java
Codecov / codecov/patch
common/src/main/java/org/opensearch/ml/common/input/nlp/TextSimilarityMLInput.java#L111