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 argument pass #1941

Merged
merged 14 commits into from
Feb 2, 2024
Merged

Fix argument pass #1941

merged 14 commits into from
Feb 2, 2024

Conversation

xinyual
Copy link
Collaborator

@xinyual xinyual commented Jan 29, 2024

Description

Fix some bug inside chatAgent and flow agent.

  1. If we want to use flow agent from chat agent, need a parse function to parse input from chat agent with key "input"
  2. Replace string.valueof with gson.toJson so we can catch map parameters
  3. A outputToOutputString used here for if output is ModelTensorOutput. It will raise error if we call toJson(output) directly and (String) output will generate the className: ModelTensorOutput instead of true value.

Issues Resolved

#1942

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.

Signed-off-by: xinyual <xinyual@amazon.com>
Signed-off-by: xinyual <xinyual@amazon.com>
Signed-off-by: xinyual <xinyual@amazon.com>
Signed-off-by: xinyual <xinyual@amazon.com>
Signed-off-by: xinyual <xinyual@amazon.com>
@xinyual xinyual had a problem deploying to ml-commons-cicd-env January 29, 2024 03:01 — with GitHub Actions Failure
@xinyual xinyual temporarily deployed to ml-commons-cicd-env January 29, 2024 03:01 — with GitHub Actions Inactive
@xinyual xinyual temporarily deployed to ml-commons-cicd-env January 29, 2024 03:01 — with GitHub Actions Inactive
@xinyual xinyual temporarily deployed to ml-commons-cicd-env January 29, 2024 03:01 — with GitHub Actions Inactive
@xinyual xinyual temporarily deployed to ml-commons-cicd-env January 29, 2024 03:01 — with GitHub Actions Inactive
@xinyual xinyual temporarily deployed to ml-commons-cicd-env January 29, 2024 03:01 — with GitHub Actions Inactive
@xinyual
Copy link
Collaborator Author

xinyual commented Jan 29, 2024

The issue is #1942

Copy link

codecov bot commented Jan 29, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (1a436fe) 82.84% compared to head (dea4cf6) 82.84%.
Report is 17 commits behind head on main.

Files Patch % Lines
...nsearch/ml/engine/algorithms/agent/AgentUtils.java 85.71% 1 Missing and 1 partial ⚠️
.../ml/engine/algorithms/agent/MLChatAgentRunner.java 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1941   +/-   ##
=========================================
  Coverage     82.84%   82.84%           
- Complexity     5444     5452    +8     
=========================================
  Files           522      522           
  Lines         21900    21924   +24     
  Branches       2226     2229    +3     
=========================================
+ Hits          18143    18164   +21     
- Misses         2848     2852    +4     
+ Partials        909      908    -1     
Flag Coverage Δ
ml-commons 82.84% <91.17%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@xinyual xinyual temporarily deployed to ml-commons-cicd-env February 2, 2024 04:01 — with GitHub Actions Inactive
@xinyual xinyual temporarily deployed to ml-commons-cicd-env February 2, 2024 04:01 — with GitHub Actions Inactive
@xinyual xinyual temporarily deployed to ml-commons-cicd-env February 2, 2024 04:01 — with GitHub Actions Inactive
@xinyual xinyual temporarily deployed to ml-commons-cicd-env February 2, 2024 04:02 — with GitHub Actions Inactive
@xinyual xinyual temporarily deployed to ml-commons-cicd-env February 2, 2024 04:02 — with GitHub Actions Inactive
@xinyual xinyual temporarily deployed to ml-commons-cicd-env February 2, 2024 04:02 — with GitHub Actions Inactive
@xinyual xinyual had a problem deploying to ml-commons-cicd-env February 2, 2024 04:28 — with GitHub Actions Failure
if (parameters.containsKey("input")) {
try {
Map<String, String> chatParameters = gson.fromJson(parameters.get("input"), Map.class);
parameters.putAll(chatParameters);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do this ? Looks it's possible to override the original params in parameters if they have same key

@@ -135,4 +138,15 @@ public String getDefaultVersion() {
return null;
}
}

private void extractFromChatParameters(Map<String, String> parameters) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this for chat only?

Signed-off-by: xinyual <xinyual@amazon.com>
@xinyual xinyual had a problem deploying to ml-commons-cicd-env February 2, 2024 07:55 — with GitHub Actions Failure
@xinyual xinyual temporarily deployed to ml-commons-cicd-env February 2, 2024 07:55 — with GitHub Actions Inactive
@xinyual xinyual temporarily deployed to ml-commons-cicd-env February 2, 2024 07:55 — with GitHub Actions Inactive
@xinyual xinyual temporarily deployed to ml-commons-cicd-env February 2, 2024 07:55 — with GitHub Actions Inactive
@xinyual xinyual temporarily deployed to ml-commons-cicd-env February 2, 2024 08:23 — with GitHub Actions Inactive
@xinyual xinyual temporarily deployed to ml-commons-cicd-env February 2, 2024 08:23 — with GitHub Actions Inactive
@xinyual xinyual temporarily deployed to ml-commons-cicd-env February 2, 2024 08:23 — with GitHub Actions Inactive
@jngz-es jngz-es merged commit 4a6ceba into opensearch-project:main Feb 2, 2024
11 of 14 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 2, 2024
* add logs

Signed-off-by: xinyual <xinyual@amazon.com>

* option2FixBug

Signed-off-by: xinyual <xinyual@amazon.com>

* remove useless log

Signed-off-by: xinyual <xinyual@amazon.com>

* remove useless log

Signed-off-by: xinyual <xinyual@amazon.com>

* fix spot less

Signed-off-by: xinyual <xinyual@amazon.com>

* change argument parsing

Signed-off-by: xinyual <xinyual@amazon.com>

* move common function to utils

Signed-off-by: xinyual <xinyual@amazon.com>

* checkout for typo

Signed-off-by: xinyual <xinyual@amazon.com>

* remove useless code

Signed-off-by: xinyual <xinyual@amazon.com>

* add UTs

Signed-off-by: xinyual <xinyual@amazon.com>

* apply spotless

Signed-off-by: xinyual <xinyual@amazon.com>

* add more uts

Signed-off-by: xinyual <xinyual@amazon.com>

* modify import

Signed-off-by: xinyual <xinyual@amazon.com>

* protect original parameters

Signed-off-by: xinyual <xinyual@amazon.com>

---------

Signed-off-by: xinyual <xinyual@amazon.com>
(cherry picked from commit 4a6ceba)
jngz-es pushed a commit that referenced this pull request Feb 2, 2024
* add logs

Signed-off-by: xinyual <xinyual@amazon.com>

* option2FixBug

Signed-off-by: xinyual <xinyual@amazon.com>

* remove useless log

Signed-off-by: xinyual <xinyual@amazon.com>

* remove useless log

Signed-off-by: xinyual <xinyual@amazon.com>

* fix spot less

Signed-off-by: xinyual <xinyual@amazon.com>

* change argument parsing

Signed-off-by: xinyual <xinyual@amazon.com>

* move common function to utils

Signed-off-by: xinyual <xinyual@amazon.com>

* checkout for typo

Signed-off-by: xinyual <xinyual@amazon.com>

* remove useless code

Signed-off-by: xinyual <xinyual@amazon.com>

* add UTs

Signed-off-by: xinyual <xinyual@amazon.com>

* apply spotless

Signed-off-by: xinyual <xinyual@amazon.com>

* add more uts

Signed-off-by: xinyual <xinyual@amazon.com>

* modify import

Signed-off-by: xinyual <xinyual@amazon.com>

* protect original parameters

Signed-off-by: xinyual <xinyual@amazon.com>

---------

Signed-off-by: xinyual <xinyual@amazon.com>
(cherry picked from commit 4a6ceba)

Co-authored-by: xinyual <74362153+xinyual@users.noreply.github.com>
austintlee pushed a commit to austintlee/ml-commons that referenced this pull request Mar 19, 2024
* add logs

Signed-off-by: xinyual <xinyual@amazon.com>

* option2FixBug

Signed-off-by: xinyual <xinyual@amazon.com>

* remove useless log

Signed-off-by: xinyual <xinyual@amazon.com>

* remove useless log

Signed-off-by: xinyual <xinyual@amazon.com>

* fix spot less

Signed-off-by: xinyual <xinyual@amazon.com>

* change argument parsing

Signed-off-by: xinyual <xinyual@amazon.com>

* move common function to utils

Signed-off-by: xinyual <xinyual@amazon.com>

* checkout for typo

Signed-off-by: xinyual <xinyual@amazon.com>

* remove useless code

Signed-off-by: xinyual <xinyual@amazon.com>

* add UTs

Signed-off-by: xinyual <xinyual@amazon.com>

* apply spotless

Signed-off-by: xinyual <xinyual@amazon.com>

* add more uts

Signed-off-by: xinyual <xinyual@amazon.com>

* modify import

Signed-off-by: xinyual <xinyual@amazon.com>

* protect original parameters

Signed-off-by: xinyual <xinyual@amazon.com>

---------

Signed-off-by: xinyual <xinyual@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants