GH-1476: Let the parsers take a caller-supplied POSTagger and Chunker

[krickert]

The chunking and tree insert parsers can tag and chunk with a POSTagger and Chunker the application supplies, instead of the POSTaggerME and ChunkerME built from the tagger and chunker models in the ParserModel.

Both parsers have a public constructor that takes a POSTagger and a Chunker next to the ParserModel. The build, attach and check models and the head rules come from the model; its tagger and chunker models are not used. The model-only constructors delegate to the new one.

ParserFactory.create(model, tagger, chunker, beamSize, advancePercentage) picks the parser for the model's ParserType with the supplied components. The existing create methods build the model's POSTaggerME and ChunkerME and call it, so the type dispatch exists once.

Arguments are checked at the public boundary with an IllegalArgumentException: a null model, tagger, chunker or head rules, a beam size below 1, and an advance percentage outside (0, 1], through ParamChecks.requirePositive and the new ParamChecks.requirePositiveFraction. The README lists the stricter checks as a breaking change, and the parser chapter of the manual shows the new factory method.

Fixes #1476

[rzo1]

Hi @krickert, thanks for the PR. Supplying an own tagger and chunker is a useful addition. Some remarks:

API

• Devil's advocate: do we need ParserBuilder? With four parameters, of which two already have defaults through ParserFactory.create, a factory overload create(model, tagger, chunker, beamSize, advancePercentage) covers the use case without a new public type, and it follows the existing factory style. A builder can still be added later in 3.x if more options come up; once released, it cannot be removed. I would suggest to drop it and keep the new constructors plus the factory overload.

Argument checks

• AbstractBottomUpParser.java: checkBeamSize and checkAdvancePercentage are local copies of generic argument checks. Please add ParamChecks.requirePositive(int, String) and a check for (0, 1] to opennlp-api and use them here. The same positive check exists in CachingStemmer, SnowballStemmerFactory, ArcStandardState, NGramLanguageModel and NGramModel, and #1323 adds another one, each with its own message. Aligning these can be a separate issue, but the new code should not add further copies.
• Please use the message format of ParamChecks instead of "beamSize must be at least 1, but was 0".

Tests

• Only the reject side is tested. Please also pin the accepted boundaries (beamSize 1, advancePercentage 1.0 and a small positive value).
• The null-argument tests can each be one @ParameterizedTest. Once the range checks live in ParamChecks, testing them there and keeping one test per entry point is sufficient.

Docs

• parser.xml: "Since 3.0.0 the parser can tag and chunk …" is changelog wording; please describe the current behaviour without the version reference.

Behaviour

• ParserFactory.create and the existing Parser constructors now reject a beam size below 1 and an advance percentage outside (0, 1]. Please add this to the README breaking-change list.

[krickert]

Hi @krickert, thanks for the PR. Supplying an own tagger and chunker is a useful addition. Some remarks:

API

• Devil's advocate: do we need ParserBuilder? With four parameters, of which two already have defaults through ParserFactory.create, a factory overload create(model, tagger, chunker, beamSize, advancePercentage) covers the use case without a new public type, and it follows the existing factory style. A builder can still be added later in 3.x if more options come up; once released, it cannot be removed. I would suggest to drop it and keep the new constructors plus the factory overload.

Maybe it's just me, but working with these constructors was a bit annoying. A builder just looked so much cleaner to me. Given that it is 3.x, and that we substantially changed parsing, I was thinking now is a good time to tear off the band aid. I considered making backward compat deprecated helpers as a compromise.. but my favor is builder pattern first..

So we don't need it, but I use parsers a lot and it might fall into "well that's your preference" territory and my personal opinion doesn't mean the standard. But builders are in fashion these days, I think our API looks better if we have it.

Large constructors are always questionable to me. 1-2 is fine, but this is larger..

However, not worried about any pushback - just thought I'd give my two cents.

I'll yield the decision to you.. it's only a few minutes to go either way.

Argument checks

• AbstractBottomUpParser.java: checkBeamSize and checkAdvancePercentage are local copies of generic argument checks. Please add ParamChecks.requirePositive(int, String) and a check for (0, 1] to opennlp-api and use them here. The same positive check exists in CachingStemmer, SnowballStemmerFactory, ArcStandardState, NGramLanguageModel and NGramModel, and OPENNLP-1991: Add a feedforward neural dependency parser #1323 adds another one, each with its own message. Aligning these can be a separate issue, but the new code should not add further copies.

np

• Please use the message format of ParamChecks instead of "beamSize must be at least 1, but was 0".

np

Tests

• Only the reject side is tested. Please also pin the accepted boundaries (beamSize 1, advancePercentage 1.0 and a small positive value).
• The null-argument tests can each be one @ParameterizedTest. Once the range checks live in ParamChecks, testing them there and keeping one test per entry point is sufficient.

on it

Docs

• parser.xml: "Since 3.0.0 the parser can tag and chunk …" is changelog wording; please describe the current behaviour without the version reference.

Sorry, I thought I got rid of those corners..

Behaviour

• ParserFactory.create and the existing Parser constructors now reject a beam size below 1 and an advance percentage outside (0, 1]. Please add this to the README breaking-change list.

np

[rzo1]

I am not a fan of mix up. Either we have builders for all features or not; I also dont value in maintaining multiple approaches (factories, etc) - we should stick with one approach only.

3.x was planned to introduce modularity. Deprecating and replacing the factory pattern with builders can be a decision but needs to be on the dev List first.

[krickert]

I am not a fan of mix up. Either we have builders for all features or not; I also dont value in maintaining multiple approaches (factories, etc) - we should stick with one approach only.

3.x was planned to introduce modularity. Deprecating and replacing the factory pattern with builders can be a decision but needs to be on the dev List first.

No problem! Thanks for clarifying. Fixes upcoming

[rzo1]

That being said: I prefer builders personally, so would be open to add them as replacement in 3.1/3.2 and remove factories in 4.x or so... whatever we decide on dev@

[krickert]

That being said: I prefer builders personally, so would be open to add them as replacement in 3.1/3.2 and remove factories in 4.x or so... whatever we decide on dev@

I'd of been surprised if you weren't. I'm in no rush, and the grpc server is natively builder OOTB.

[krickert]

Thanks, done in the new push:

• Dropped ParserBuilder; the new ParserFactory.create(model, tagger, chunker, beamSize, advancePercentage) covers the use case.
• Added ParamChecks.requirePositive(int, String) (same signature as OPENNLP-1991: Add a feedforward neural dependency parser #1323) and requirePositiveFraction(double, String) for (0, 1], both tested in ParamChecksTest; AbstractBottomUpParser uses them. The other copies are left for a separate issue as suggested.
• One @ParameterizedTest per entry point for nulls and one for out-of-range values, plus accept rows for beam size 1 and advance percentage 1.0 and 0.01.
• parser.xml describes the current behaviour without the version.
• README lists the stricter beam size and advance percentage checks as breaking.