Skip to content

Commit

Permalink
allow multiple ab-tests & add ab-test for grpc rule (#9164)
Browse files Browse the repository at this point in the history
* allow multiple ab-tests & add ab-test for ggec

* support for ab-test property in remote rule options

* support "no-regression" option for grpc rule
  • Loading branch information
SteVio89 authored Aug 28, 2023
1 parent 7dcd4a6 commit 568c3b5
Show file tree
Hide file tree
Showing 6 changed files with 49 additions and 23 deletions.
14 changes: 14 additions & 0 deletions languagetool-core/src/main/java/org/languagetool/Language.java
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,20 @@ public List<Rule> getRelevantRemoteRules(ResourceBundle messageBundle, List<Remo
.filter(config -> config.getRuleId().startsWith("TEST"))
.map(c -> new TestRemoteRule(this, c))
.forEach(rules::add);
if (userConfig.getAbTest() != null) {
rules.removeIf(rule -> {
String activeRemoteRuleAbTest = ((RemoteRule) rule).getServiceConfiguration().getOptions().get("abtest"); //abtest option value must match the abtest value from server.properties
if (activeRemoteRuleAbTest != null && !activeRemoteRuleAbTest.trim().isEmpty()) {
if (userConfig.getAbTest().contains(activeRemoteRuleAbTest)) { // A/B-Test is active for remote rule and user is enabled for A/B-Test
return false;
} else { // A/B-Test is active for remote rule and user is disabled for A/B-Test
return true; //
}
} else {
return false; // No A/B-Test active for remote rule
}
});
}
return rules;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public static boolean hasABTestsEnabled() {
// partially indifferent for comparing UserConfigs (e.g. in PipelinePool)
// provided to rules only for A/B tests
private final Long textSessionId;
private final String abTest;
private final List<String> abTest;
private final String preferredLanguages;

public UserConfig() {
Expand Down Expand Up @@ -93,7 +93,7 @@ public UserConfig(List<String> userSpecificSpellerWords,
int maxSpellingSuggestions, Long premiumUid, String userDictName,
Long userDictCacheSize,
LinguServices linguServices, boolean filterDictionaryMatches,
@Nullable String abTest, @Nullable Long textSessionId, boolean hidePremiumMatches, List<String> preferredLanguages) {
@Nullable List<String> abTest, @Nullable Long textSessionId, boolean hidePremiumMatches, List<String> preferredLanguages) {
this.userSpecificSpellerWords = Objects.requireNonNull(userSpecificSpellerWords);
this.userSpecificRules = Objects.requireNonNull(userSpecificRules);
for (Map.Entry<String, Integer> entry : ruleValues.entrySet()) {
Expand Down Expand Up @@ -268,7 +268,7 @@ public Long getTextSessionId() {
return textSessionId;
}

public String getAbTest() {
public List<String> getAbTest() {
return abTest;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import java.io.File;
import java.io.IOException;
import java.net.MalformedURLException;
import java.net.URI;
import java.net.URL;
import java.util.ArrayList;
import java.util.Collections;
Expand All @@ -50,7 +49,6 @@
import com.google.common.util.concurrent.ListenableFuture;

import io.grpc.*;
import io.grpc.internal.DnsNameResolver;
import io.grpc.internal.DnsNameResolverProvider;
import org.jetbrains.annotations.Nullable;
import org.languagetool.AnalyzedSentence;
Expand Down Expand Up @@ -206,7 +204,6 @@ public GRPCRule(Language language, ResourceBundle messages, RemoteRuleConfig con
.equalsIgnoreCase("true");
this.batchSize = Integer.parseInt(config.getOptions().getOrDefault("batchSize",
String.valueOf(DEFAULT_BATCH_SIZE)));

synchronized (servers) {
Connection conn = null;
try {
Expand All @@ -221,10 +218,12 @@ public GRPCRule(Language language, ResourceBundle messages, RemoteRuleConfig con
protected class MLRuleRequest extends RemoteRule.RemoteRequest {
final List<MLServerProto.MatchRequest> requests;
final List<AnalyzedSentence> sentences;
final Long textSessionId;

public MLRuleRequest(List<MLServerProto.MatchRequest> requests, List<AnalyzedSentence> sentences) {
public MLRuleRequest(List<MLServerProto.MatchRequest> requests, List<AnalyzedSentence> sentences, Long textSessionId) {
this.requests = requests;
this.sentences = sentences;
this.textSessionId = textSessionId;
}
}

Expand Down Expand Up @@ -286,7 +285,7 @@ protected RemoteRule.RemoteRequest prepareRequest(List<AnalyzedSentence> sentenc
if (requests.size() > 1) {
logger.debug("Split {} sentences into {} requests for {}", sentences.size(), requests.size(), getId());
}
return new MLRuleRequest(requests, sentences);
return new MLRuleRequest(requests, sentences, textSessionId);
}
}

Expand All @@ -301,6 +300,13 @@ private static String nonEmpty(String s) {
@Override
protected Callable<RemoteRuleResult> executeRequest(RemoteRequest requestArg, long timeoutMilliseconds) throws TimeoutException {
return () -> {
MLRuleRequest reqArgs = (MLRuleRequest) requestArg;
// NOTE: disabled for now, don't want to run this in the nightly diff
boolean noRegression = Boolean.parseBoolean(serviceConfiguration.getOptions().getOrDefault("no-regression", "false"));
if (noRegression && (reqArgs.textSessionId == -1 || reqArgs.textSessionId == -2)) {
return new RemoteRuleResult(false, true, Collections.emptyList(), reqArgs.sentences);
}

List<AnalyzedSentence> sentences;
List<ListenableFuture<MatchResponse>> futures = new ArrayList<>();
List<MatchResponse> responses = new ArrayList<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,4 @@ protected int getPriorityForId(String id) {
public boolean hasMinMatchesRules() {
return true;
}


}
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ public void setRestrictManagedAccounts(boolean restrictManagedAccounts) {

protected int slowRuleLoggingThreshold = -1; // threshold in milliseconds, used by SlowRuleLogger; < 0 - disabled

protected String abTest = null;
protected List<String> abTest = null;
protected Pattern abTestClients = null;
protected int abTestRollout = 100; // percentage [0,100]
protected File ngramLangIdentData;
Expand Down Expand Up @@ -1276,22 +1276,21 @@ boolean isStoppable() {

/**
* @since 4.4
* See if a specific A/B-Test is to be run
* Get a list of active A/B-Tests
*/
@Nullable
public String getAbTest() {
public List<String> getAbTest() {
return abTest;
}

/**
* @since 4.4
* Enable a specific A/B-Test to be run (or null to disable all tests)
* Enable A/B-Tests to be run (comma seperated for a list or null to disable all tests)
*/
public void setAbTest(@Nullable String abTest) {
if (abTest != null && abTest.trim().isEmpty()) {
this.abTest = null;
if (abTest != null && !abTest.trim().isEmpty()) {
this.abTest = new ArrayList<>(Arrays.asList(abTest.trim().split(",")));
} else {
this.abTest = abTest;
this.abTest = Collections.emptyList();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -354,8 +354,9 @@ void checkText(AnnotatedText aText, HttpExchange httpExchange, Map<String, Strin
", HTTP user agent: " + getHttpUserAgent(httpExchange) + ", referrer: " + getHttpReferrer(httpExchange));
}

String abTest = null;
List<String> abTest = null;
if (agent != null && config.getAbTestClients() != null && config.getAbTestClients().matcher(agent).matches()) {
//TODO: it is not possible to have individual AbTestClients per AbTest
boolean testRolledOut;
// partial rollout; deterministic if textSessionId given to make testing easier
if (textSessionId != null) {
Expand All @@ -364,14 +365,22 @@ void checkText(AnnotatedText aText, HttpExchange httpExchange, Map<String, Strin
testRolledOut = random.nextInt(100) < config.getAbTestRollout();
}
if (testRolledOut) {
abTest = config.getAbTest();
abTest = Collections.unmodifiableList(config.getAbTest());
}
}
String paramActivatedAbTest = params.get("abtest");
if (paramActivatedAbTest != null && paramActivatedAbTest.equals(config.getAbTest())) {
abTest = paramActivatedAbTest;
if (paramActivatedAbTest != null) {
String[] abParams = paramActivatedAbTest.trim().split(",");
List<String> tmpAb = new ArrayList<>();
for (String abParam : abParams) {
if (config.getAbTest().contains(abParam)) {
tmpAb.add(abParam.trim());
}
}
if (!tmpAb.isEmpty()) {
abTest = Collections.unmodifiableList(tmpAb);
}
}


boolean enableHiddenRules = "true".equals(params.get("enableHiddenRules"));
if (limits.hasPremium()) {
Expand Down

0 comments on commit 568c3b5

Please sign in to comment.