Skip to content

Commit 3f04902

Browse files
zane-neozane-neo
authored
Remove ppl tool execution setting (#381)
* Remove ppl tool execution setting Signed-off-by: zane-neo <[email protected]> * fix failure UTs Signed-off-by: zane-neo <[email protected]> * fix failure ITs Signed-off-by: zane-neo <[email protected]> --------- Signed-off-by: zane-neo <[email protected]>
1 parent 5a9dbcd commit 3f04902

File tree

7 files changed

+5
-131
lines changed

7 files changed

+5
-131
lines changed

‎src/main/java/org/opensearch/agent/ToolPlugin.java‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
import java.util.List;
1111
import java.util.function.Supplier;
1212

13-
import org.opensearch.agent.common.SkillSettings;
1413
import org.opensearch.agent.tools.CreateAlertTool;
1514
import org.opensearch.agent.tools.CreateAnomalyDetectorTool;
1615
import org.opensearch.agent.tools.NeuralSparseSearchTool;
@@ -21,12 +20,9 @@
2120
import org.opensearch.agent.tools.SearchAnomalyResultsTool;
2221
import org.opensearch.agent.tools.SearchMonitorsTool;
2322
import org.opensearch.agent.tools.VectorDBTool;
24-
import org.opensearch.agent.tools.utils.ClusterSettingHelper;
2523
import org.opensearch.client.Client;
2624
import org.opensearch.cluster.metadata.IndexNameExpressionResolver;
2725
import org.opensearch.cluster.service.ClusterService;
28-
import org.opensearch.common.settings.Setting;
29-
import org.opensearch.common.settings.Settings;
3026
import org.opensearch.core.common.io.stream.NamedWriteableRegistry;
3127
import org.opensearch.core.xcontent.NamedXContentRegistry;
3228
import org.opensearch.env.Environment;
@@ -65,9 +61,7 @@ public Collection<Object> createComponents(
6561
this.client = client;
6662
this.clusterService = clusterService;
6763
this.xContentRegistry = xContentRegistry;
68-
Settings settings = environment.settings();
69-
ClusterSettingHelper clusterSettingHelper = new ClusterSettingHelper(settings, clusterService);
70-
PPLTool.Factory.getInstance().init(client, clusterSettingHelper);
64+
PPLTool.Factory.getInstance().init(client);
7165
NeuralSparseSearchTool.Factory.getInstance().init(client, xContentRegistry);
7266
VectorDBTool.Factory.getInstance().init(client, xContentRegistry);
7367
RAGTool.Factory.getInstance().init(client, xContentRegistry);
@@ -97,8 +91,4 @@ public List<Tool.Factory<? extends Tool>> getToolFactories() {
9791
);
9892
}
9993

100-
@Override
101-
public List<Setting<?>> getSettings() {
102-
return List.of(SkillSettings.PPL_EXECUTION_ENABLED);
103-
}
10494
}

‎src/main/java/org/opensearch/agent/common/SkillSettings.java‎

Lines changed: 0 additions & 22 deletions
This file was deleted.

‎src/main/java/org/opensearch/agent/tools/PPLTool.java‎

Lines changed: 3 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,6 @@
3131
import org.opensearch.action.ActionRequest;
3232
import org.opensearch.action.admin.indices.mapping.get.GetMappingsRequest;
3333
import org.opensearch.action.search.SearchRequest;
34-
import org.opensearch.agent.common.SkillSettings;
35-
import org.opensearch.agent.tools.utils.ClusterSettingHelper;
3634
import org.opensearch.agent.tools.utils.ToolHelper;
3735
import org.opensearch.client.Client;
3836
import org.opensearch.cluster.metadata.MappingMetadata;
@@ -98,9 +96,7 @@ public class PPLTool implements Tool {
9896

9997
private int head;
10098

101-
private ClusterSettingHelper clusterSettingHelper;
102-
103-
private static Gson gson = new Gson();
99+
private static Gson gson = org.opensearch.ml.common.utils.StringUtils.gson;
104100

105101
private static Map<String, String> DEFAULT_PROMPT_DICT;
106102

@@ -153,7 +149,6 @@ public static PPLModelType from(String value) {
153149

154150
public PPLTool(
155151
Client client,
156-
ClusterSettingHelper clusterSettingHelper,
157152
String modelId,
158153
String contextPrompt,
159154
String pplModelType,
@@ -172,7 +167,6 @@ public PPLTool(
172167
this.previousToolKey = previousToolKey;
173168
this.head = head;
174169
this.execute = execute;
175-
this.clusterSettingHelper = clusterSettingHelper;
176170
}
177171

178172
@SuppressWarnings("unchecked")
@@ -227,14 +221,7 @@ public <T> void run(Map<String, String> parameters, ActionListener<T> listener)
227221
return;
228222
}
229223
String ppl = parseOutput(dataAsMap.get("response"), indexName);
230-
boolean pplExecutedEnabled = clusterSettingHelper.getClusterSettings(SkillSettings.PPL_EXECUTION_ENABLED);
231-
if (!pplExecutedEnabled || !this.execute) {
232-
if (!pplExecutedEnabled) {
233-
log
234-
.debug(
235-
"PPL execution is disabled, the query will be returned directly, to enable this, please set plugins.skills.ppl_execution_enabled to true"
236-
);
237-
}
224+
if (!this.execute) {
238225
Map<String, String> ret = ImmutableMap.of("ppl", ppl);
239226
listener.onResponse((T) AccessController.doPrivileged((PrivilegedExceptionAction<String>) () -> gson.toJson(ret)));
240227
return;
@@ -303,8 +290,6 @@ public boolean validate(Map<String, String> parameters) {
303290
public static class Factory implements Tool.Factory<PPLTool> {
304291
private Client client;
305292

306-
private ClusterSettingHelper clusterSettingHelper;
307-
308293
private static Factory INSTANCE;
309294

310295
public static Factory getInstance() {
@@ -320,17 +305,15 @@ public static Factory getInstance() {
320305
}
321306
}
322307

323-
public void init(Client client, ClusterSettingHelper clusterSettingHelper) {
308+
public void init(Client client) {
324309
this.client = client;
325-
this.clusterSettingHelper = clusterSettingHelper;
326310
}
327311

328312
@Override
329313
public PPLTool create(Map<String, Object> map) {
330314
validatePPLToolParameters(map);
331315
return new PPLTool(
332316
client,
333-
clusterSettingHelper,
334317
(String) map.get("model_id"),
335318
(String) map.getOrDefault("prompt", ""),
336319
(String) map.getOrDefault("model_type", ""),

‎src/main/java/org/opensearch/agent/tools/utils/ClusterSettingHelper.java‎

Lines changed: 0 additions & 35 deletions
This file was deleted.

‎src/test/java/org/opensearch/agent/tools/PPLToolTests.java‎

Lines changed: 1 addition & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,13 @@
99
import static org.mockito.ArgumentMatchers.any;
1010
import static org.mockito.ArgumentMatchers.eq;
1111
import static org.mockito.Mockito.doAnswer;
12-
import static org.mockito.Mockito.mock;
1312
import static org.mockito.Mockito.when;
1413
import static org.opensearch.ml.common.CommonValue.ML_CONNECTOR_INDEX;
1514
import static org.opensearch.ml.common.utils.StringUtils.gson;
1615

1716
import java.util.Collections;
1817
import java.util.HashMap;
1918
import java.util.Map;
20-
import java.util.Set;
2119

2220
import org.apache.lucene.search.TotalHits;
2321
import org.junit.Before;
@@ -26,15 +24,10 @@
2624
import org.mockito.MockitoAnnotations;
2725
import org.opensearch.action.admin.indices.mapping.get.GetMappingsResponse;
2826
import org.opensearch.action.search.SearchResponse;
29-
import org.opensearch.agent.common.SkillSettings;
30-
import org.opensearch.agent.tools.utils.ClusterSettingHelper;
3127
import org.opensearch.client.AdminClient;
3228
import org.opensearch.client.Client;
3329
import org.opensearch.client.IndicesAdminClient;
3430
import org.opensearch.cluster.metadata.MappingMetadata;
35-
import org.opensearch.cluster.service.ClusterService;
36-
import org.opensearch.common.settings.ClusterSettings;
37-
import org.opensearch.common.settings.Settings;
3831
import org.opensearch.core.action.ActionListener;
3932
import org.opensearch.core.common.bytes.BytesArray;
4033
import org.opensearch.core.common.bytes.BytesReference;
@@ -128,13 +121,7 @@ public void setup() {
128121
listener.onResponse(transportPPLQueryResponse);
129122
return null;
130123
}).when(client).execute(eq(PPLQueryAction.INSTANCE), any(), any());
131-
132-
Settings settings = Settings.builder().put(SkillSettings.PPL_EXECUTION_ENABLED.getKey(), true).build();
133-
ClusterService clusterService = mock(ClusterService.class);
134-
when(clusterService.getSettings()).thenReturn(settings);
135-
when(clusterService.getClusterSettings()).thenReturn(new ClusterSettings(settings, Set.of(SkillSettings.PPL_EXECUTION_ENABLED)));
136-
ClusterSettingHelper clusterSettingHelper = new ClusterSettingHelper(settings, clusterService);
137-
PPLTool.Factory.getInstance().init(client, clusterSettingHelper);
124+
PPLTool.Factory.getInstance().init(client);
138125
}
139126

140127
@Test
@@ -449,26 +436,6 @@ public void testTool_executePPLFailure() {
449436
);
450437
}
451438

452-
@Test
453-
public void test_pplTool_whenPPLExecutionDisabled_returnOnlyContainsPPL() {
454-
Settings settings = Settings.builder().put(SkillSettings.PPL_EXECUTION_ENABLED.getKey(), false).build();
455-
ClusterService clusterService = mock(ClusterService.class);
456-
when(clusterService.getSettings()).thenReturn(settings);
457-
when(clusterService.getClusterSettings()).thenReturn(new ClusterSettings(settings, Set.of(SkillSettings.PPL_EXECUTION_ENABLED)));
458-
ClusterSettingHelper clusterSettingHelper = new ClusterSettingHelper(settings, clusterService);
459-
PPLTool.Factory.getInstance().init(client, clusterSettingHelper);
460-
PPLTool tool = PPLTool.Factory
461-
.getInstance()
462-
.create(ImmutableMap.of("model_id", "modelId", "prompt", "contextPrompt", "head", "100"));
463-
assertEquals(PPLTool.TYPE, tool.getName());
464-
465-
tool.run(ImmutableMap.of("index", "demo", "question", "demo"), ActionListener.<String>wrap(executePPLResult -> {
466-
Map<String, String> returnResults = gson.fromJson(executePPLResult, Map.class);
467-
assertNull(returnResults.get("executionResult"));
468-
assertEquals("source=demo| head 1", returnResults.get("ppl"));
469-
}, log::error));
470-
}
471-
472439
private void createMappings() {
473440
indexMappings = new HashMap<>();
474441
indexMappings

‎src/test/java/org/opensearch/integTest/BaseAgentToolsIT.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,6 @@ public void updateClusterSettings() {
6565
updateClusterSettings("plugins.ml_commons.jvm_heap_memory_threshold", 100);
6666
updateClusterSettings("plugins.ml_commons.allow_registering_model_via_url", true);
6767
updateClusterSettings("plugins.ml_commons.agent_framework_enabled", true);
68-
updateClusterSettings("plugins.skills.ppl_execution_enabled", true);
6968
}
7069

7170
@SneakyThrows

‎src/test/java/org/opensearch/integTest/PPLToolIT.java‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -58,14 +58,6 @@ public void testPPLTool() {
5858
);
5959
}
6060

61-
public void test_PPLTool_whenPPLExecutionDisabled_ResultOnlyContainsPPL() {
62-
updateClusterSettings("plugins.skills.ppl_execution_enabled", false);
63-
prepareIndex();
64-
String agentId = registerAgent();
65-
String result = executeAgent(agentId, "{\"parameters\": {\"question\": \"correct\", \"index\": \"employee\"}}");
66-
assertEquals("{\"ppl\":\"source\\u003demployee| where age \\u003e 56 | stats COUNT() as cnt\"}", result);
67-
}
68-
6961
public void testPPLTool_withWrongPPLGenerated_thenThrowException() {
7062
prepareIndex();
7163
String agentId = registerAgent();

0 commit comments

Comments
 (0)