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

Add feature flag for QueryPhaseSearcher #214

Merged
merged 10 commits into from
Jul 14, 2023
+103 −40
Merged

Add feature flag for QueryPhaseSearcher #214

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1fa7e78
Adding hybrid_search_enabled settings
martin-gaievski Jul 1, 2023
4985c33
Add feature flag at plugin level
martin-gaievski Jul 6, 2023
3fabc54
Adopt existing tests to latest changes in knn
martin-gaievski Jul 12, 2023
55780e6
Making all method arguments final
martin-gaievski Jul 12, 2023
5142d3f
Refactor feature name, remove plugin feature flags class
martin-gaievski Jul 12, 2023
c1356cf
Adopt recent refactoring in core
martin-gaievski Jul 12, 2023
2617ca4
Invert feature flag meaning, disable feature by default
martin-gaievski Jul 13, 2023
a63b983
Fixed more copilation errors after latest changes in core
martin-gaievski Jul 13, 2023
a6c52bb
Remove setting of flag from plugin, make false default value
martin-gaievski Jul 14, 2023
86b51f5
Minor refactoring in uni tests
martin-gaievski Jul 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,8 @@ testClusters.integTest {
// Increase heap size from default of 512mb to 1gb. When heap size is 512mb, our integ tests sporadically fail due
// to ml-commons memory circuit breaker exception
jvmArgs("-Xms1g", "-Xmx1g")
// enable hybrid search for testing
systemProperty('neural_search_hybrid_search_enabled', 'true')
}

// Remote Integration Tests
Expand Down
24 changes: 5 additions & 19 deletions src/main/java/org/opensearch/neuralsearch/plugin/NeuralSearch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import java.util.Optional;
import java.util.function.Supplier;

import org.apache.commons.lang3.tuple.Pair;
import org.opensearch.client.Client;
import org.opensearch.cluster.metadata.IndexNameExpressionResolver;
import org.opensearch.cluster.service.ClusterService;
Expand All @@ -39,7 +38,6 @@
import org.opensearch.script.ScriptService;
import org.opensearch.search.query.QueryPhaseSearcher;
import org.opensearch.threadpool.ThreadPool;
import org.opensearch.transport.TransportSettings;
import org.opensearch.watcher.ResourceWatcherService;

import com.google.common.annotations.VisibleForTesting;
Expand All @@ -52,15 +50,9 @@ public class NeuralSearch extends Plugin implements ActionPlugin, SearchPlugin,
* Gates the functionality of hybrid search
* Currently query phase searcher added with hybrid search will conflict with concurrent search in core.
* Once that problem is resolved this feature flag can be removed.
* Key is the name string, value is key + transport feature specific prefix,
* prefix is added by core when we register feature (https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/plugins/PluginsService.java#L277)
* We need to write and read by the value, key is only for definition
*/
@VisibleForTesting
public static final Pair<String, String> NEURAL_SEARCH_HYBRID_SEARCH_DISABLED = Pair.of(
"neural_search_hybrid_search_disabled",
String.join(".", TransportSettings.FEATURE_PREFIX, "neural_search_hybrid_search_disabled")
);
public static final String NEURAL_SEARCH_HYBRID_SEARCH_ENABLED = "neural_search_hybrid_search_enabled";
Copy link
Collaborator

Choose a reason for hiding this comment

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

The variable is already public we can remove @VisbleForTesting

Copy link
Member Author

@martin-gaievski martin-gaievski Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I made it public only because it's required for tests otherwise private will be enough, my understanding is that @VisbleForTesting is required in this case to mark that it's intentional.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh.. sorry my bad. I read it wrong then

private MLCommonsClientAccessor clientAccessor;

@Override
Expand Down Expand Up @@ -97,16 +89,10 @@ public Map<String, Processor.Factory> getProcessors(Processor.Parameters paramet

@Override
public Optional<QueryPhaseSearcher> getQueryPhaseSearcher() {
if (FeatureFlags.isEnabled(NEURAL_SEARCH_HYBRID_SEARCH_DISABLED.getValue())) {
// we want feature be disabled by default due to risk of colliding and breaking concurrent search in core
// but we only can set "true" values by default due to implementation limitations in core.
return Optional.empty();
if (FeatureFlags.isEnabled(NEURAL_SEARCH_HYBRID_SEARCH_ENABLED)) {
return Optional.of(new HybridQueryPhaseSearcher());
}
return Optional.of(new HybridQueryPhaseSearcher());
}

@Override
protected Optional<String> getFeature() {
return Optional.of(NEURAL_SEARCH_HYBRID_SEARCH_DISABLED.getKey());
// we want feature be disabled by default due to risk of colliding and breaking concurrent search in core
return Optional.empty();
}
}
14 changes: 3 additions & 11 deletions src/test/java/org/opensearch/neuralsearch/plugin/NeuralSearchTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
package org.opensearch.neuralsearch.plugin;

import static org.mockito.Mockito.mock;
import static org.opensearch.neuralsearch.plugin.NeuralSearch.NEURAL_SEARCH_HYBRID_SEARCH_DISABLED;
import static org.opensearch.neuralsearch.plugin.NeuralSearch.NEURAL_SEARCH_HYBRID_SEARCH_ENABLED;

import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -42,15 +42,15 @@ public void testQueryPhaseSearcher() {
assertNotNull(queryPhaseSearcher);
assertTrue(queryPhaseSearcher.isEmpty());

System.setProperty(NEURAL_SEARCH_HYBRID_SEARCH_DISABLED.getValue(), "true");
System.setProperty(NEURAL_SEARCH_HYBRID_SEARCH_ENABLED, "true");
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we set this property in base class?

and why test are extending OpenSearchTestCase, there is should be a base Neural Search test class

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking that for some unit tests we may want to disable it, that would be complex if it's set in base class.

I'll make setting in a method and will call it in a setup phase for this test class.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure


Optional<QueryPhaseSearcher> queryPhaseSearcherWithFeatureFlagDisabled = plugin.getQueryPhaseSearcher();

assertNotNull(queryPhaseSearcherWithFeatureFlagDisabled);
assertFalse(queryPhaseSearcherWithFeatureFlagDisabled.isEmpty());
assertTrue(queryPhaseSearcherWithFeatureFlagDisabled.get() instanceof HybridQueryPhaseSearcher);

System.setProperty(NEURAL_SEARCH_HYBRID_SEARCH_DISABLED.getValue(), "");
System.setProperty(NEURAL_SEARCH_HYBRID_SEARCH_ENABLED, "");
}

public void testProcessors() {
Expand All @@ -60,12 +60,4 @@ public void testProcessors() {
assertNotNull(processors);
assertNotNull(processors.get(TextEmbeddingProcessor.TYPE));
}

public void testFeature() {
NeuralSearch plugin = new NeuralSearch();
Optional<String> feature = plugin.getFeature();
assertNotNull(feature);
assertFalse(feature.isEmpty());
assertEquals(NEURAL_SEARCH_HYBRID_SEARCH_DISABLED.getKey(), feature.get());
}
}
4 changes: 4 additions & 0 deletions src/test/java/org/opensearch/neuralsearch/search/query/HybridQueryPhaseSearcherTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ public void testQueryType_whenQueryIsHybrid_thenCallHybridDocCollector() {
IndexShard indexShard = mock(IndexShard.class);
when(indexShard.shardId()).thenReturn(new ShardId("test", "test", 0));
when(searchContext.indexShard()).thenReturn(indexShard);
when(searchContext.bucketCollectorProcessor()).thenReturn(SearchContext.NO_OP_BUCKET_COLLECTOR_PROCESSOR);

LinkedList<QueryCollectorContext> collectors = new LinkedList<>();
boolean hasFilterCollector = randomBoolean();
Expand Down Expand Up @@ -191,6 +192,7 @@ public void testQueryType_whenQueryIsNotHybrid_thenDoNotCallHybridDocCollector()
when(indexShard.shardId()).thenReturn(new ShardId("test", "test", 0));
when(searchContext.indexShard()).thenReturn(indexShard);
when(searchContext.queryResult()).thenReturn(new QuerySearchResult());
when(searchContext.bucketCollectorProcessor()).thenReturn(SearchContext.NO_OP_BUCKET_COLLECTOR_PROCESSOR);

LinkedList<QueryCollectorContext> collectors = new LinkedList<>();
boolean hasFilterCollector = randomBoolean();
Expand Down Expand Up @@ -260,6 +262,7 @@ public void testQueryResult_whenOneSubQueryWithHits_thenHybridResultsAreSet() {
when(searchContext.indexShard()).thenReturn(indexShard);
QuerySearchResult querySearchResult = new QuerySearchResult();
when(searchContext.queryResult()).thenReturn(querySearchResult);
when(searchContext.bucketCollectorProcessor()).thenReturn(SearchContext.NO_OP_BUCKET_COLLECTOR_PROCESSOR);

LinkedList<QueryCollectorContext> collectors = new LinkedList<>();
boolean hasFilterCollector = randomBoolean();
Expand Down Expand Up @@ -350,6 +353,7 @@ public void testQueryResult_whenMultipleTextSubQueriesWithSomeHits_thenHybridRes
IndexShard indexShard = mock(IndexShard.class);
when(indexShard.shardId()).thenReturn(new ShardId("test", "test", 0));
when(searchContext.indexShard()).thenReturn(indexShard);
when(searchContext.bucketCollectorProcessor()).thenReturn(SearchContext.NO_OP_BUCKET_COLLECTOR_PROCESSOR);

LinkedList<QueryCollectorContext> collectors = new LinkedList<>();
boolean hasFilterCollector = randomBoolean();
Expand Down