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

Revert "Added Conversation API in MLClient" #3632

Merged
merged 1 commit into from
Mar 8, 2025
Merged

Revert "Added Conversation API in MLClient" #3632

merged 1 commit into from
Mar 8, 2025

Conversation

owaiskazi19
Copy link
Member

Reverts #3475 to unblock Flow Framework

@owaiskazi19
Revert "Added Conversation API in MLClient (opensearch-project#3475)"
aee05b3 
This reverts commit 627980a.

Signed-off-by: Owais <owaiskazi19@gmail.com>
@owaiskazi19 owaiskazi19 temporarily deployed to ml-commons-cicd-env-require-approval March 8, 2025 00:08 — with GitHub Actions Inactive
@owaiskazi19 owaiskazi19 temporarily deployed to ml-commons-cicd-env-require-approval March 8, 2025 00:08 — with GitHub Actions Inactive
@owaiskazi19 owaiskazi19 had a problem deploying to ml-commons-cicd-env-require-approval March 8, 2025 00:08 — with GitHub Actions Error
@owaiskazi19 owaiskazi19 had a problem deploying to ml-commons-cicd-env-require-approval March 8, 2025 00:08 — with GitHub Actions Failure
@owaiskazi19
Copy link
Member Author

Looks like the issue is from the upstream core

| class: org.apache.lucene.codecs.lucene90.Lucene90DocValuesConsumerWrapper
| jar1: /Users/kazabdu/.gradle/caches/modules-2/files-2.1/org.opensearch/opensearch/3.0.0-alpha1-SNAPSHOT/ca0d2b98c3e4748966e1e8bd784b5af016b187f7/opensearch-3.0.0-alpha1-SNAPSHOT.jar
| jar2: /Users/kazabdu/.gradle/caches/modules-2/files-2.1/org.opensearch/opensearch-ml-client/3.0.0.0-alpha1-SNAPSHOT/459c26e6ce20883267dcf225e8bffc434e6c9566/opensearch-ml-client-3.0.0.0-alpha1-SNAPSHOT.jar
|       at org.opensearch.common.bootstrap.JarHell.checkClass(JarHell.java:316)
|       at org.opensearch.common.bootstrap.JarHell.checkJarHell(JarHell.java:215)
|       at org.opensearch.common.bootstrap.JarHell.checkJarHell(JarHell.java:102)
|       at org.opensearch.common.bootstrap.JarHell.main(JarHell.java:86)

Let's not merge this until we decide on that

@jngz-es
Copy link
Collaborator

jngz-es commented Mar 8, 2025 •
edited
Loading

Hi @owaiskazi19 , it turns out some API breaking changes which would break any use case using it. Usually we have deprecation plan at first, then we could remove it. I don't think we should remove APIs directly.
And also could you provide more details about how it is blocking Flow framework to help us understand the issue?

@owaiskazi19
Copy link
Member Author

Hi @owaiskazi19 , it turns out some API breaking changes which would break any use case using it. Usually we have deprecation plan at first, then we could remove it. I don't think we should remove APIs directly. And also could you provide more details about how it is blocking Flow framework?

This is the error and the API was added today so no dependencies
https://github.com/opensearch-project/flow-framework/actions/runs/13729568900/job/38403720785?pr=1074

@jngz-es
Copy link
Collaborator

jngz-es commented Mar 8, 2025

Got it, it is a pretty new pr needed reverting. I am OK with that. BTW it's a jarhell issue, why you don't solve http-core dependency but remove some new added methods?

@mingshl
Copy link
Collaborator

mingshl commented Mar 8, 2025

let's merge this PR first to avoid the jar hell issue. When you test it fully, let's raise another new PR to bring back add conversation API @owaiskazi19

@mingshl mingshl merged commit 2193b0f into opensearch-project:main Mar 8, 2025
8 of 10 checks passed
akolarkunnu pushed a commit to akolarkunnu/ml-commons that referenced this pull request Mar 11, 2025
@owaiskazi19 @akolarkunnu
Revert "Added Conversation API in MLClient (opensearch-project#3475)" (…
4240810 
…opensearch-project#3632)

This reverts commit 627980a.

Signed-off-by: Owais <owaiskazi19@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jngz-es jngz-es jngz-es approved these changes

@mingshl mingshl mingshl approved these changes

@b4sjoo b4sjoo Awaiting requested review from b4sjoo b4sjoo is a code owner

@dhrubo-os dhrubo-os Awaiting requested review from dhrubo-os dhrubo-os is a code owner

@model-collapse model-collapse Awaiting requested review from model-collapse model-collapse is a code owner

@rbhavna rbhavna Awaiting requested review from rbhavna rbhavna is a code owner

@ylwu-amzn ylwu-amzn Awaiting requested review from ylwu-amzn ylwu-amzn is a code owner

@zane-neo zane-neo Awaiting requested review from zane-neo zane-neo is a code owner

@Zhangxunmt Zhangxunmt Awaiting requested review from Zhangxunmt Zhangxunmt is a code owner

@austintlee austintlee Awaiting requested review from austintlee austintlee is a code owner

@HenryL27 HenryL27 Awaiting requested review from HenryL27 HenryL27 is a code owner

@xinyual xinyual Awaiting requested review from xinyual xinyual is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@owaiskazi19 @jngz-es @mingshl