Copilot commented on code in PR #3174:
URL: https://github.com/apache/tika/pull/3174#discussion_r4018303305
##########
docs/modules/ROOT/pages/configuration/parsers/vlm-parsers.adoc:
##########
@@ -29,8 +29,9 @@ for earlier shapes.
Three implementations are provided out of the box. None is auto-loaded: each
must be
named explicitly in your configuration. (Changed in 4.1.0: `openai-vlm-parser`
previously
auto-registered via SPI.) To use a VLM as the OCR engine for embedded images
and rendered
-PDF pages, name it in the `text-recognizers` list — see
-xref:configuration/index.adoc[Configuration].
+PDF pages, configure it under `engines` and name it in the `text-recognizers`
list — see
+xref:configuration/index.adoc[Configuration]. Naming it under `parsers` is
deprecated since
+4.1.0 and unsupported in 4.2.0.
Review Comment:
The `engines` form shown here cannot load these VLM classes: `EngineLoader`
requires `Engine`, while the VLM implementations are parser-based
`Parser`/`TextRecognizer` components. The page now directs users to an invalid
configuration; document the inline `text-recognizers` form (or add an adapter)
instead.
##########
tika-parsers/tika-parsers-ml/tika-inference/src/main/java/org/apache/tika/inference/OpenAIImageEmbeddingParser.java:
##########
@@ -185,9 +190,10 @@ public void parse(TikaInputStream tis, ContentHandler
handler,
@Override
public void initialize() throws TikaConfigException {
- LOG.info("openai-image-embedding-parser runs one request per image;
the \"engines\" + "
- + "\"inference\" shape (openai-embedding-engine, input IMAGES,
task embed) batches "
- + "a document's images into one request");
+ LOG.warn("openai-image-embedding-parser is deprecated since 4.1.0 and
will be removed in "
+ + "4.2.0: configure the endpoint as an openai-embedding-engine
under \"engines\" "
+ + "and bind it with an IMAGES \"inference\" binding (task
embed), which batches a "
+ + "document's images into one request");
Review Comment:
For a legacy `openai-image-embedding-parser` entry, `ComponentInstantiator`
invokes this initializer and `ParserLoader.finish()` now emits another
deprecation WARN for the same entry. That contradicts the one-WARN-per-entry
behavior documented in the release notes; centralize the warning or suppress
one of these paths.
##########
tika-serialization/src/main/java/org/apache/tika/config/loader/ParserLoader.java:
##########
@@ -205,66 +205,71 @@ private static String names(List<Parser> parsers) {
return names.toString();
}
- /**
- * Enrichers named directly under {@code "parsers"} that the composite
never dispatches
- * to: every type they advertise is claimed by another parser there, or
they advertise
- * none (engine unavailable, or told to skip). Both shapes look configured
and do
- * nothing as parsers.
- */
- static List<Parser> undispatchedEnrichers(Parser root) {
- List<Parser> inert = new ArrayList<>();
+ /** Engines (enrichers) named directly under {@code "parsers"}: the
deprecated 4.0 shape. */
+ static List<Parser> enginesUnderParsers(Parser root) {
+ List<Parser> engines = new ArrayList<>();
if (!(root instanceof CompositeParser composite) || root instanceof
DefaultParser) {
- return inert;
+ return engines;
+ }
+ for (Parser member : composite.getAllComponentParsers()) {
+ if (ContentEnrichers.isEnricher(member)) {
+ engines.add(member);
+ }
+ }
+ return engines;
+ }
+
+ /** The types the composite dispatches to this member as their parser. */
+ static Set<MediaType> parsedTypes(Parser root, Parser member) {
+ Set<MediaType> parsed = new TreeSet<>();
+ if (!(root instanceof CompositeParser composite)) {
+ return parsed;
}
ParseContext empty = new ParseContext();
Map<MediaType, Parser> dispatch = composite.getParsers(empty);
MediaTypeRegistry registry = composite.getMediaTypeRegistry();
- for (Parser member : composite.getAllComponentParsers()) {
- if (!ContentEnrichers.isEnricher(member)) {
- continue;
- }
- boolean dispatched = false;
- for (MediaType type : member.getSupportedTypes(empty)) {
- if (dispatch.get(registry.normalize(type)) == member) {
- dispatched = true;
- break;
- }
- }
- if (!dispatched) {
- inert.add(member);
+ for (MediaType type : member.getSupportedTypes(empty)) {
+ if (dispatch.get(registry.normalize(type)) == member) {
+ parsed.add(type);
}
}
- return inert;
+ return parsed;
}
- // the 4.0 shape still works, so it is INFO; an entry that never runs at
all is a WARN
- private static void logUndispatched(Parser inert, CompositeContentEnricher
enrichers,
- boolean listConfigured) {
- String name = ParserUtils.getParserClassname(inert);
- Set<MediaType> advertised = inert.getSupportedTypes(new
ParseContext());
+ /** One WARN per engine under "parsers": the deprecation, then what the
entry does today. */
+ private static void warnEngineUnderParsers(Parser engine, Parser root,
+ CompositeContentEnricher
enrichers,
+ boolean listConfigured) {
+ String name = ParserUtils.getParserClassname(engine);
+ String lead = name + " is named under \"parsers\", which is deprecated
for engines since "
+ + "4.1.0 and unsupported in 4.2.0: configure it under
\"engines\" and name it in "
+ + "\"text-recognizers\". ";
Review Comment:
This migration advice is invalid for the OCR/VLM parser classes that this
warning handles: `engines` accepts only `Engine` implementations, while those
classes are `Parser`/`ContentEnricher` implementations. Following the warning
makes the replacement fail config loading; direct parser-based recognizers need
the inline `text-recognizers` form unless an Engine adapter is added.
##########
tika-parsers/tika-parsers-ml/tika-inference/src/main/java/org/apache/tika/inference/OpenAIImageEmbeddingParser.java:
##########
@@ -80,7 +80,12 @@
* {@link #parse}.
*
* @since Apache Tika 4.0
+ * @deprecated since 4.1.0, removed in 4.2.0. Configure the endpoint as an
+ * {@code openai-embedding-engine} under {@code "engines"} and bind it with an
{@code IMAGES}
+ * {@code "inference"} binding and the {@code embed} task: one request per
document tree
+ * instead of one per image, and the vectors land where {@link ChunkTarget}
puts them.
Review Comment:
The replacement `IMAGES` binding batches by
`EmbeddingEngine.getMaxBatchSize()`, so documents with more images than that
limit still make multiple requests. This new Javadoc promises one request per
document tree; describe it as bounded batching instead.
This issue also appears on line 195 of the same file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
