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

First step towards supporting templating in rank eval requests. #20374

Merged
merged 23 commits into from Nov 1, 2016

Conversation

Projects
None yet
5 participants
@MaineC
Copy link
Contributor

commented Sep 7, 2016

Relates to #20231

Caveats:

  • This does not yet reflect exactly the request API w/ template as discussed in
  • One of the yaml tests this comes with does not yet pass (namely 30_template.yaml), currently I'm scratching my head whether this should even be there. If the answer is "no" I'll continue scratching my head how to best test this, would love to have some sort of integration test. Posting for early feedback.

I tried writing a REST test to show the whole thing actually works. While this was helpful in finding some request parsing issues, the test currently doesn't work as intended as no specific mustache integration is on the classpath for the module. Also I feel like I'm replicating some of the request parsing logic we already have for template search requests. Would be great if @martijnvg could
have a look and provide some feedback.

Isabel Drost-Fromm
First step towards supporting templating in rank eval requests.
Relates to #20231

Caveats:

This does not yet reflect exactly the request API w/ template as discussed in

One of the yaml tests this comes with does not yet pass (namely
30_template.yaml), currently I'm scratching my head whether this should even be
there. If the answer is "no" I'll continue scratching my head how to best test
this, would love to have some sort of integration test.
Posting for early feedback.

I tried writing a REST test to show the whole thing actually works. While this
was helpful in finding some request parsing issues, the test currently doesn't
work as intended as no specific mustache integration is on the classpath for the
module. Also I feel like I'm replicating some of the request parsing logic we
already have for template search requests. Would be great if @martijnvg could
have a look and provide some feedback.
@martijnvg

This comment has been minimized.

Copy link
Member

commented Sep 8, 2016

@MaineC If you like to test templating in combination rank eval api then you need to do this in a qa module. We do the same for example for ingest which supports templating too. There is a smoke-test-ingest-with-all-dependencies qa module. You could add smoke-test-rank-eval-with-mustache module that just runs the rest tests that are testing with mustache templating.

@SuppressWarnings({ "unchecked", "rawtypes" })
Map<String, String> params = (Map) query_spec.getParams();
Script script = new Script(template, ScriptService.ScriptType.INLINE, "mustache", params);
String resolvedRequest =

This comment has been minimized.

Copy link
@martijnvg

martijnvg Sep 8, 2016

Member

I think we should turn this code in a util method on ScriptService in core module. There are now several places where we have code similar to this. This would fix the code duplication.

Something like:

public SearchSourceBuilder templateSearchRequest(Script script, QueryParseContext context) {
   ....
}
"requests" : [
{
"id": "amsterdam_query",
"params": { "query_string": "amsterdam_1" },

This comment has been minimized.

Copy link
@clintongormley

clintongormley Sep 9, 2016

Member

Why do you use amsterdam_1 and berlin_1 in the query, when the docs contain amsterdam and berlin?

"ratings": [{"key": {"index": "foo", "type": "bar", "doc_id": "doc1"}, "rating": 1}]
}
],
"metric" : { "precisionatn": { "size": 10}}

This comment has been minimized.

Copy link
@clintongormley

clintongormley Sep 9, 2016

Member

precision_atn would be more readable

@MaineC

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2016

Hi @martijnvg, I tried adding the templating REST test in a separate qa module (see the cdf13d8 commit).

I'm able to run the smoke-test-ingest-with-all-dependencies separately by cd'ing into that directory and executing gradle check. When doing the same with the added smoke-test-rank-eval-with-mustache I get the following error:

* Where:
Build file '/home/isabel/projects/es/source/elasticsearch/qa/smoke-test-rank-eval-with-    mustache/build.gradle' line: 20

* What went wrong:
A problem occurred evaluating root project 'smoke-test-rank-eval-with-mustache'.
> Plugin with id 'elasticsearch.rest-test' not found.

I'm sure I missed adding some dependency somewhere, however comparing the build.gradle file in my qa module to the one in the ingest qa module didn't help me in figuring out what I missed. Maybe you can help me out here.

@rjernst

This comment has been minimized.

Copy link
Member

commented Sep 22, 2016

@MaineC You would get that error if qa:smoke-test-rank-eval-with-mustache did not exist in settings.gradle.

@MaineC

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2016

@rjernst I definitely didn't touch that one, so I guess this is the issue. Thanks for the speedy response.

@MaineC MaineC force-pushed the MaineC:feature/rank-eval-template branch to 0d56cd2 Oct 18, 2016

@MaineC

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2016

@cbuescher would be great if you could have a look at this.

@clintongormley thanks for spotting the typo in the rest test, I guess that was a debugging leftover. @rjernst thanks for the hint about how to get this built, that worked.

@martijnvg thanks for the hint to put the integration test into the qa module, turns out going through this test uncovered several issues with my first implementation. I fully agree with your comment on moving part of the parsing to a separate helper, but I think this is something for a separate PR? Would be great if you could take a look to make sure the approach taken makes sense in general.

@martijnvg

This comment has been minimized.

Copy link
Member

commented Oct 18, 2016

@MaineC Agreed, that this should happen in a different PR. I can do this.

@MaineC MaineC added v6.0.0-alpha1 and removed WIP labels Oct 19, 2016

@cbuescher cbuescher self-assigned this Oct 19, 2016

@cbuescher
Copy link
Member

left a comment

This looks great in terms of functionality, I left a few minor ideas about naming and some comments regarding code. RankEvalRequestTests fails for me locally, I left a comment where I think the culprit is.

@@ -85,7 +99,7 @@ public RankedListQualityMetric getEvaluator() {
}

/** Sets the precision at n configuration (containing level of n to consider).*/
public void setEvaluator(RankedListQualityMetric config) {
public void setEvaluator(RankedListQualityMetric config) { // TODO rename to metric, remove duplicate setEval

This comment has been minimized.

Copy link
@cbuescher

cbuescher Oct 19, 2016

Member

+1 can we do that in this PR or is that a bigger change? Also looks like the comment is a bit off here.

@@ -98,7 +112,18 @@ public void setEvaluator(RankedListQualityMetric config) {
public void setSpecifications(Collection<RatedRequest> specifications) {
this.ratedRequests = specifications;
}

/** Set the script to base test requests on. */
public void setScript(Script script) {

This comment has been minimized.

Copy link
@cbuescher

cbuescher Oct 19, 2016

Member

nit: naming this "template" instead of script (although the argument is technically a Script) could communicate the usage of this setting better


private static final ParseField SCRIPT_FIELD = new ParseField("script");

This comment has been minimized.

Copy link
@cbuescher

cbuescher Oct 19, 2016

Member

nit: same as above, from a user perspective naming this "script" might create the impression that we can do arbitrary stuff here, maybe call it "template"?

} catch (IOException ex) {
throw new ParsingException(p.getTokenLocation(), "error parsing rank request", ex);
}
}, SCRIPT_FIELD);

This comment has been minimized.

Copy link
@cbuescher

cbuescher Oct 19, 2016

Member

I recently used the Script.parse methof in the same way when refactoring the ScriptSortParser, Nik encouraged be to add a static 'parse' method to the Script class that does the wrapping of the potential IOException. This can most likely also be used here.

This comment has been minimized.

Copy link
@MaineC

MaineC Oct 20, 2016

Author Contributor

If you are referring to https://github.com/elastic/elasticsearch/blob/master/core/src/main/java/org/elasticsearch/script/Script.java#L194 ... this needs a QueryParseContext as parameter, all I have is a RankEvalParseContext.

After chatting on Slack we decided to change the signature of the method to take a ParseFieldMatcher and the ScriptLanguage so that fits for this use case as well.

Turns out this was the original parsing method's signature. So simply removed the method @cbuescher added, inserted a try/catch in the original parsing method instead.

This comment has been minimized.

Copy link
@MaineC

MaineC Oct 20, 2016

Author Contributor

See commit 1a33476 once pushed

RankEvalSpec spec = PARSER.parse(parser, context);

if (templated) {
logger.trace("template: {}", spec.script);

This comment has been minimized.

Copy link
@cbuescher

cbuescher Oct 19, 2016

Member

asked f2f, we can probably delete logging at this point.

This comment has been minimized.

Copy link
@MaineC

MaineC Oct 20, 2016

Author Contributor

done

@@ -93,9 +93,9 @@ public void testRoundtripping() throws IOException {

This comment has been minimized.

Copy link
@cbuescher

cbuescher Oct 19, 2016

Member

Can we add the new "script" parameter to the randomized setup for this test?


/** Collection of query specifications, that is e.g. search request templates to use for query translation. */
private Collection<RatedRequest> ratedRequests = new ArrayList<>();
/** Definition of the quality metric, e.g. precision at N */
private RankedListQualityMetric metric;
/** optional: Template to base test requests on */
private Script script;

This comment has been minimized.

Copy link
@cbuescher

cbuescher Oct 19, 2016

Member

For lack of a better line to comment this: I think script is missing from hashCode/equals

This comment has been minimized.

Copy link
@MaineC

MaineC Oct 19, 2016

Author Contributor

Darn, thanks for spotting that.

@@ -50,6 +56,8 @@
private List<String> summaryFields = new ArrayList<>();
/** Collection of rated queries for this query QA specification.*/
private List<RatedDocument> ratedDocs = new ArrayList<>();
/** Map of parameters to use for filling a query template, can be used instead of providing testRequest. */
private Map<String, Object> params = new HashMap<>();

This comment has been minimized.

Copy link
@cbuescher

cbuescher Oct 19, 2016

Member

I think this is missing from hashCode/Equals as well

This comment has been minimized.

Copy link
@MaineC

MaineC Oct 20, 2016

Author Contributor

Thanks for spotting

@@ -73,6 +86,7 @@ public void writeTo(StreamOutput out) throws IOException {
spec.writeTo(out);
}
out.writeNamedWriteable(metric);
script.writeTo(out);

This comment has been minimized.

Copy link
@cbuescher

cbuescher Oct 19, 2016

Member

script seems to be optional, I get NPEs in some roundtrip tests for this.

@@ -64,6 +76,7 @@ public RankEvalSpec(StreamInput in) throws IOException {
ratedRequests.add(new RatedRequest(in));
}
metric = in.readNamedWriteable(RankedListQualityMetric.class);
script = new Script(in);

This comment has been minimized.

Copy link
@cbuescher

cbuescher Oct 19, 2016

Member

this can blow up when there was no script writen to the StreamInput

Isabel Drost-Fromm
Fix failing unit test: Serialisation issue.
When script wasn't set, this would previously cause a npe

Isabel Drost-Fromm added some commits Oct 19, 2016

Isabel Drost-Fromm
Isabel Drost-Fromm
This removes the IOException catching version of parsing @cbuescher a…
…dded

earlier in favor of catching the exception in the parsing code itself as
suggested by review.
Isabel Drost-Fromm
Isabel Drost-Fromm
Isabel Drost-Fromm
@MaineC

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2016

@cbuescher I finally got the last test failure ironed out today. Can you take another look to check that your comments have been addressed appropriately?

@cbuescher
Copy link
Member

left a comment

Thanks, LGTM. I left two nitpicking things, feel free to add or leave as you like. I'd just squash down the rather detailed commit history here before merging.

template.writeTo(out);
} else {
out.writeBoolean(false);
}

This comment has been minimized.

Copy link
@cbuescher

cbuescher Oct 31, 2016

Member

nit: writeOptionalWriteable() would be a bit shorter

@@ -64,6 +74,9 @@ public RankEvalSpec(StreamInput in) throws IOException {
ratedRequests.add(new RatedRequest(in));
}
metric = in.readNamedWriteable(RankedListQualityMetric.class);
if (in.readBoolean()) {
template = new Script(in);
}

This comment has been minimized.

Copy link
@cbuescher

cbuescher Oct 31, 2016

Member

nit: in.readOptionalWriteable() would be a bit shorter

@MaineC MaineC merged commit 0b8a2e4 into elastic:feature/rank-eval Nov 1, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.