From 0ea88032465fb2b9eff35fec1815cb99baa8b06a Mon Sep 17 00:00:00 2001 From: "opensearch-trigger-bot[bot]" <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Date: Sat, 3 Aug 2024 10:19:49 +0800 Subject: [PATCH] [Backport] Remove ppl tool execution setting (#383) (#384) * Remove ppl tool execution setting * fix failure UTs * backport 381 to 2.x --------- (cherry picked from commit fc9ae9399b835d70a6d9f6ec1ebf8394abbcafff) Signed-off-by: zane-neo Signed-off-by: github-actions[bot] Co-authored-by: github-actions[bot] --- .../java/org/opensearch/agent/ToolPlugin.java | 12 +------ .../agent/common/SkillSettings.java | 22 ------------ .../org/opensearch/agent/tools/PPLTool.java | 23 ++---------- .../tools/utils/ClusterSettingHelper.java | 35 ------------------- .../opensearch/agent/tools/PPLToolTests.java | 35 +------------------ .../integTest/BaseAgentToolsIT.java | 1 - .../org/opensearch/integTest/PPLToolIT.java | 8 ----- 7 files changed, 5 insertions(+), 131 deletions(-) delete mode 100644 src/main/java/org/opensearch/agent/common/SkillSettings.java delete mode 100644 src/main/java/org/opensearch/agent/tools/utils/ClusterSettingHelper.java diff --git a/src/main/java/org/opensearch/agent/ToolPlugin.java b/src/main/java/org/opensearch/agent/ToolPlugin.java index 74ff6bf4..ac0aa484 100644 --- a/src/main/java/org/opensearch/agent/ToolPlugin.java +++ b/src/main/java/org/opensearch/agent/ToolPlugin.java @@ -10,7 +10,6 @@ import java.util.List; import java.util.function.Supplier; -import org.opensearch.agent.common.SkillSettings; import org.opensearch.agent.tools.CreateAnomalyDetectorTool; import org.opensearch.agent.tools.NeuralSparseSearchTool; import org.opensearch.agent.tools.PPLTool; @@ -20,12 +19,9 @@ import org.opensearch.agent.tools.SearchAnomalyResultsTool; import org.opensearch.agent.tools.SearchMonitorsTool; import org.opensearch.agent.tools.VectorDBTool; -import org.opensearch.agent.tools.utils.ClusterSettingHelper; import org.opensearch.client.Client; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.cluster.service.ClusterService; -import org.opensearch.common.settings.Setting; -import org.opensearch.common.settings.Settings; import org.opensearch.core.common.io.stream.NamedWriteableRegistry; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.env.Environment; @@ -64,9 +60,7 @@ public Collection createComponents( this.client = client; this.clusterService = clusterService; this.xContentRegistry = xContentRegistry; - Settings settings = environment.settings(); - ClusterSettingHelper clusterSettingHelper = new ClusterSettingHelper(settings, clusterService); - PPLTool.Factory.getInstance().init(client, clusterSettingHelper); + PPLTool.Factory.getInstance().init(client); NeuralSparseSearchTool.Factory.getInstance().init(client, xContentRegistry); VectorDBTool.Factory.getInstance().init(client, xContentRegistry); RAGTool.Factory.getInstance().init(client, xContentRegistry); @@ -94,8 +88,4 @@ public List> getToolFactories() { ); } - @Override - public List> getSettings() { - return List.of(SkillSettings.PPL_EXECUTION_ENABLED); - } } diff --git a/src/main/java/org/opensearch/agent/common/SkillSettings.java b/src/main/java/org/opensearch/agent/common/SkillSettings.java deleted file mode 100644 index 55808748..00000000 --- a/src/main/java/org/opensearch/agent/common/SkillSettings.java +++ /dev/null @@ -1,22 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.agent.common; - -import org.opensearch.common.settings.Setting; - -/** - * Settings for skills plugin - */ -public final class SkillSettings { - - private SkillSettings() {} - - /** - * This setting controls whether PPL execution is enabled or not - */ - public static final Setting PPL_EXECUTION_ENABLED = Setting - .boolSetting("plugins.skills.ppl_execution_enabled", false, Setting.Property.NodeScope, Setting.Property.Dynamic); -} diff --git a/src/main/java/org/opensearch/agent/tools/PPLTool.java b/src/main/java/org/opensearch/agent/tools/PPLTool.java index 8a8d09f7..621426ac 100644 --- a/src/main/java/org/opensearch/agent/tools/PPLTool.java +++ b/src/main/java/org/opensearch/agent/tools/PPLTool.java @@ -31,8 +31,6 @@ import org.opensearch.action.ActionRequest; import org.opensearch.action.admin.indices.mapping.get.GetMappingsRequest; import org.opensearch.action.search.SearchRequest; -import org.opensearch.agent.common.SkillSettings; -import org.opensearch.agent.tools.utils.ClusterSettingHelper; import org.opensearch.agent.tools.utils.ToolHelper; import org.opensearch.client.Client; import org.opensearch.cluster.metadata.MappingMetadata; @@ -98,9 +96,7 @@ public class PPLTool implements Tool { private int head; - private ClusterSettingHelper clusterSettingHelper; - - private static Gson gson = new Gson(); + private static Gson gson = org.opensearch.ml.common.utils.StringUtils.gson; private static Map DEFAULT_PROMPT_DICT; @@ -153,7 +149,6 @@ public static PPLModelType from(String value) { public PPLTool( Client client, - ClusterSettingHelper clusterSettingHelper, String modelId, String contextPrompt, String pplModelType, @@ -172,7 +167,6 @@ public PPLTool( this.previousToolKey = previousToolKey; this.head = head; this.execute = execute; - this.clusterSettingHelper = clusterSettingHelper; } @SuppressWarnings("unchecked") @@ -222,14 +216,7 @@ public void run(Map parameters, ActionListener listener) ModelTensor modelTensor = modelTensors.getMlModelTensors().get(0); Map dataAsMap = (Map) modelTensor.getDataAsMap(); String ppl = parseOutput(dataAsMap.get("response"), indexName); - boolean pplExecutedEnabled = clusterSettingHelper.getClusterSettings(SkillSettings.PPL_EXECUTION_ENABLED); - if (!pplExecutedEnabled || !this.execute) { - if (!pplExecutedEnabled) { - log - .debug( - "PPL execution is disabled, the query will be returned directly, to enable this, please set plugins.skills.ppl_execution_enabled to true" - ); - } + if (!this.execute) { Map ret = ImmutableMap.of("ppl", ppl); listener.onResponse((T) AccessController.doPrivileged((PrivilegedExceptionAction) () -> gson.toJson(ret))); return; @@ -298,8 +285,6 @@ public boolean validate(Map parameters) { public static class Factory implements Tool.Factory { private Client client; - private ClusterSettingHelper clusterSettingHelper; - private static Factory INSTANCE; public static Factory getInstance() { @@ -315,9 +300,8 @@ public static Factory getInstance() { } } - public void init(Client client, ClusterSettingHelper clusterSettingHelper) { + public void init(Client client) { this.client = client; - this.clusterSettingHelper = clusterSettingHelper; } @Override @@ -325,7 +309,6 @@ public PPLTool create(Map map) { validatePPLToolParameters(map); return new PPLTool( client, - clusterSettingHelper, (String) map.get("model_id"), (String) map.getOrDefault("prompt", ""), (String) map.getOrDefault("model_type", ""), diff --git a/src/main/java/org/opensearch/agent/tools/utils/ClusterSettingHelper.java b/src/main/java/org/opensearch/agent/tools/utils/ClusterSettingHelper.java deleted file mode 100644 index 92bf9dcd..00000000 --- a/src/main/java/org/opensearch/agent/tools/utils/ClusterSettingHelper.java +++ /dev/null @@ -1,35 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.agent.tools.utils; - -import java.util.Optional; - -import org.opensearch.cluster.service.ClusterService; -import org.opensearch.common.settings.Setting; -import org.opensearch.common.settings.Settings; - -import lombok.AllArgsConstructor; - -/** - * This class is to encapsulate the {@link Settings} and {@link ClusterService} and provide a general method to retrieve dynamical cluster settings conveniently. - */ -@AllArgsConstructor -public class ClusterSettingHelper { - - private Settings settings; - - private ClusterService clusterService; - - /** - * Retrieves the cluster settings for the specified setting. - * - * @param setting the setting to retrieve cluster settings for - * @return the cluster setting value, or the default setting value if not found - */ - public T getClusterSettings(Setting setting) { - return Optional.ofNullable(clusterService.getClusterSettings().get(setting)).orElse(setting.get(settings)); - } -} diff --git a/src/test/java/org/opensearch/agent/tools/PPLToolTests.java b/src/test/java/org/opensearch/agent/tools/PPLToolTests.java index 8e2c3aaa..ae1baa31 100644 --- a/src/test/java/org/opensearch/agent/tools/PPLToolTests.java +++ b/src/test/java/org/opensearch/agent/tools/PPLToolTests.java @@ -9,7 +9,6 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import static org.opensearch.ml.common.CommonValue.ML_CONNECTOR_INDEX; import static org.opensearch.ml.common.utils.StringUtils.gson; @@ -17,7 +16,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.Map; -import java.util.Set; import org.apache.lucene.search.TotalHits; import org.junit.Before; @@ -26,15 +24,10 @@ import org.mockito.MockitoAnnotations; import org.opensearch.action.admin.indices.mapping.get.GetMappingsResponse; import org.opensearch.action.search.SearchResponse; -import org.opensearch.agent.common.SkillSettings; -import org.opensearch.agent.tools.utils.ClusterSettingHelper; import org.opensearch.client.AdminClient; import org.opensearch.client.Client; import org.opensearch.client.IndicesAdminClient; import org.opensearch.cluster.metadata.MappingMetadata; -import org.opensearch.cluster.service.ClusterService; -import org.opensearch.common.settings.ClusterSettings; -import org.opensearch.common.settings.Settings; import org.opensearch.core.action.ActionListener; import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.common.bytes.BytesReference; @@ -128,13 +121,7 @@ public void setup() { listener.onResponse(transportPPLQueryResponse); return null; }).when(client).execute(eq(PPLQueryAction.INSTANCE), any(), any()); - - Settings settings = Settings.builder().put(SkillSettings.PPL_EXECUTION_ENABLED.getKey(), true).build(); - ClusterService clusterService = mock(ClusterService.class); - when(clusterService.getSettings()).thenReturn(settings); - when(clusterService.getClusterSettings()).thenReturn(new ClusterSettings(settings, Set.of(SkillSettings.PPL_EXECUTION_ENABLED))); - ClusterSettingHelper clusterSettingHelper = new ClusterSettingHelper(settings, clusterService); - PPLTool.Factory.getInstance().init(client, clusterSettingHelper); + PPLTool.Factory.getInstance().init(client); } @Test @@ -413,26 +400,6 @@ public void testTool_executePPLFailure() { ); } - @Test - public void test_pplTool_whenPPLExecutionDisabled_returnOnlyContainsPPL() { - Settings settings = Settings.builder().put(SkillSettings.PPL_EXECUTION_ENABLED.getKey(), false).build(); - ClusterService clusterService = mock(ClusterService.class); - when(clusterService.getSettings()).thenReturn(settings); - when(clusterService.getClusterSettings()).thenReturn(new ClusterSettings(settings, Set.of(SkillSettings.PPL_EXECUTION_ENABLED))); - ClusterSettingHelper clusterSettingHelper = new ClusterSettingHelper(settings, clusterService); - PPLTool.Factory.getInstance().init(client, clusterSettingHelper); - PPLTool tool = PPLTool.Factory - .getInstance() - .create(ImmutableMap.of("model_id", "modelId", "prompt", "contextPrompt", "head", "100")); - assertEquals(PPLTool.TYPE, tool.getName()); - - tool.run(ImmutableMap.of("index", "demo", "question", "demo"), ActionListener.wrap(executePPLResult -> { - Map returnResults = gson.fromJson(executePPLResult, Map.class); - assertNull(returnResults.get("executionResult")); - assertEquals("source=demo| head 1", returnResults.get("ppl")); - }, log::error)); - } - private void createMappings() { indexMappings = new HashMap<>(); indexMappings diff --git a/src/test/java/org/opensearch/integTest/BaseAgentToolsIT.java b/src/test/java/org/opensearch/integTest/BaseAgentToolsIT.java index 853a2974..658a3fc7 100644 --- a/src/test/java/org/opensearch/integTest/BaseAgentToolsIT.java +++ b/src/test/java/org/opensearch/integTest/BaseAgentToolsIT.java @@ -63,7 +63,6 @@ public void updateClusterSettings() { updateClusterSettings("plugins.ml_commons.jvm_heap_memory_threshold", 100); updateClusterSettings("plugins.ml_commons.allow_registering_model_via_url", true); updateClusterSettings("plugins.ml_commons.agent_framework_enabled", true); - updateClusterSettings("plugins.skills.ppl_execution_enabled", true); } @SneakyThrows diff --git a/src/test/java/org/opensearch/integTest/PPLToolIT.java b/src/test/java/org/opensearch/integTest/PPLToolIT.java index b208e1f2..46cbd864 100644 --- a/src/test/java/org/opensearch/integTest/PPLToolIT.java +++ b/src/test/java/org/opensearch/integTest/PPLToolIT.java @@ -58,14 +58,6 @@ public void testPPLTool() { ); } - public void test_PPLTool_whenPPLExecutionDisabled_ResultOnlyContainsPPL() { - updateClusterSettings("plugins.skills.ppl_execution_enabled", false); - prepareIndex(); - String agentId = registerAgent(); - String result = executeAgent(agentId, "{\"parameters\": {\"question\": \"correct\", \"index\": \"employee\"}}"); - assertEquals("{\"ppl\":\"source\\u003demployee| where age \\u003e 56 | stats COUNT() as cnt\"}", result); - } - public void testPPLTool_withWrongPPLGenerated_thenThrowException() { prepareIndex(); String agentId = registerAgent();