OPENNLP-124: Maxent/Perceptron training should report progess back via an API #758

NishantShri4 · 2025-03-24T07:01:35Z

Thank you for contributing to Apache OpenNLP.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with OPENNLP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit?

For code changes:

Have you ensured that the full suite of tests is executed via mvn clean install at the root opennlp folder?
Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file, including the main LICENSE file in opennlp folder?
If applicable, have you updated the NOTICE file, including the main NOTICE file found in opennlp folder?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check GitHub Actions for build issues and submit an update to your PR as soon as possible.

NishantShri4 · 2025-03-24T07:27:15Z

Hello @mawiesne / @kottmann . Good day!

Is this item waiting to be picked up? https://issues.apache.org/jira/browse/OPENNLP-124.
Here is my draft PR.
This PR is just to explain my current understanding of the Jira .
This needs further improvements (refactoring, integration with other model trainers, unit tests etc.).
I am happy to be part of the discussion and pick it up for implementation if the team approves.
Pls. let me know your views.

Attached the output of a couple of existing Tests (Perceptron Trainer) based on the integration with Console based TrainingProgressMonitor.
PerceptroTrainerUnitTests-LogOutput.txt

mawiesne · 2025-03-24T07:33:27Z

FYI: @jzonthemtn + @rzo1

rzo1

Thanks for the draft. I added some thoughts / comments.

opennlp-tools/src/main/java/opennlp/tools/monitoring/ConsoleTrainingProgressMonitor.java

...lp-tools/src/main/java/opennlp/tools/monitoring/PrevNIterationAccuracyLessThanTolerance.java

opennlp-tools/src/main/java/opennlp/tools/monitoring/StopCriteria.java

opennlp-tools/src/main/java/opennlp/tools/monitoring/TrainingProgressMonitor.java

opennlp-tools/src/main/java/opennlp/tools/ml/perceptron/PerceptronTrainer.java

NishantShri4 · 2025-04-13T23:17:56Z

hi @rzo1, @mawiesne,

Hope you are well. I have tried to fix the review comments. Would you pls. be able to review once more and direct towards the intended solution? Many thanks in advance.

Some queries and ToDos:

The jira description gives following prototype of finishedTraining() method.

finishedTraining(int iterations, int numberCorrectEvents, int totalEvents, StopCriteria stopCriteria);
}

Please can you clarify, what is the use of numberCorrectEvents and totalEvents parameters? In my current implementation, I have not used them, instead I found stopCriteria is sufficient. Pls. take a look.

ToDo : Update the documentation to include description of TPM and StopCriteria.

NishantShri4 · 2025-04-14T22:34:59Z

opennlp-tools/src/main/java/opennlp/tools/monitoring/StopCriteria.java

+/**
+ * Stop-criteria for a {@link opennlp.tools.ml.model.AbstractModel} training .
+ */
+public interface StopCriteria extends Predicate<Double> {


This can be made as extends Predicate<Object>;
So that this common API is flexible to receive any object as input to verify testing Criteria? views pls.

@NishantShri4

Or even nicer
public interface StopCriteria<T extends Number> extends Predicate<T> {

This way, several numerical stop criteria can be defined, either as integer/long or float/double precision.
Don't think that we need sth different than Number as base type for T, do we?

NishantShri4 · 2025-04-15T05:55:37Z

opennlp-tools/src/main/java/opennlp/tools/monitoring/TrainingProgressMonitor.java

+   * used to store the training progress events. Callers of this method can invoke it periodically
+   * during training progress to avoid storing too much progress related data.
+   */
+  void displayAndClear();


This can be made to accept a boolean input parameter which decides whether the underlying data structure is cleared or not?

If parameter clear is introduced, method should be renamed to void display(boolean clear)

opennlp-tools/src/test/java/opennlp/tools/monitoring/DefaultTrainingProgressMonitorTest.java

NishantShri4 · 2025-04-16T17:36:00Z

Hi Reviewers - All checks are green now. This is available for review. If possible, pls. take a look.
One query - This PR has 10 commits. Before presenting it for review, Is it needed to club them into a single commit?

mawiesne · 2025-04-16T18:34:25Z

Hi Reviewers - All checks are green now. This is available for review. If possible, pls. take a look.

One query - This PR has 10 commits. Before presenting it for review, Is it needed to club them into a single commit?

Thx @NishantShri4 for moving this topic forward! Could you squash those commits and force push the resulting single commit? Once available, we'll have a detailed look and provide feedback.

…ia an API

NishantShri4 · 2025-04-16T23:00:16Z

Thanks @mawiesne. This is done (rebase, squash and force push).

mawiesne

Thx @NishantShri4 for providing this substantial contribution. I've left feedback by comments to further improve it. Once addressed, I'll re-check and potentially, @rzo1 can add his final thoughts/checks then.

mawiesne · 2025-04-21T08:01:10Z

opennlp-tools/pom.xml

@@ -72,6 +72,13 @@
      <scope>test</scope>
    </dependency>

+    <dependency>
+      <groupId>org.assertj</groupId>
+      <artifactId>assertj-core</artifactId>


@NishantShri4 Can we avoid AssertJ as extra dependency? I wonder, why assertions of JUnit 5.x are not sufficient?

mawiesne · 2025-04-21T08:04:33Z

opennlp-tools/src/main/java/opennlp/tools/commons/Trainer.java

+   *
+   * @param trainParams The {@link TrainingParameters} to use.
+   * @param reportMap The {@link Map} instance used as report map.
+   * @param config The {@link TrainingConfiguration} to use.


The training configuration should not be null at runtime, I guess?

Pls therefore add:
"Must not be {@code null}." to the parameter JavaDoc.

Same applies for different scenarios, pls re-iterate JavaDoc with that idea in mind.

mawiesne · 2025-04-21T08:05:41Z

opennlp-tools/src/main/java/opennlp/tools/ml/AbstractTrainer.java

+   *
+   * @param trainParams The {@link TrainingParameters} to use.
+   * @param reportMap The {@link Map} instance used as report map.
+   * @param config The {@link TrainingConfiguration} to use.


See comment for Trainer interface.

mawiesne · 2025-04-21T08:06:26Z

opennlp-tools/src/main/java/opennlp/tools/ml/AbstractTrainer.java

@@ -108,4 +124,12 @@ protected void addToReport(String key, String value) {
    reportMap.put(key, value);
  }

+  /**
+   * Retrieves the {@link TrainingConfiguration} associated with a {@link AbstractTrainer}.


Should read "with an...", extra "n"

mawiesne · 2025-04-21T08:07:25Z

opennlp-tools/src/main/java/opennlp/tools/ml/TrainerFactory.java

+   * and a null {@link opennlp.tools.monitoring.StopCriteria}.
+   * If not provided, the actual {@link opennlp.tools.monitoring.StopCriteria}
+   * will be decided by the {@link EventTrainer} implementation.
+   *


Pls add specific parameter JavaDoc.
Pls add specific return value JavaDoc.

mawiesne · 2025-04-21T08:29:02Z

opennlp-tools/src/test/java/opennlp/tools/monitoring/DefaultTrainingProgressMonitorTest.java

+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.read.ListAppender;
+import org.assertj.core.api.Assertions;


Try to eliminate dependance on org.assertj.core... here and elsewhere.

mawiesne · 2025-04-21T08:29:48Z

opennlp-tools/src/test/java/opennlp/tools/monitoring/IterDeltaAccuracyUnderToleranceTest.java

+  }
+
+  @ParameterizedTest()
+  @CsvSource( {"0.01,false", "-0.01,false", "0.00001,true", "-0.00001,true"})


That's a nice approach! 👍

mawiesne · 2025-04-21T08:30:41Z

opennlp-tools/src/test/java/opennlp/tools/monitoring/IterDeltaAccuracyUnderToleranceTest.java

+        .00002)));
+  }
+
+  @ParameterizedTest()


Remove extra brackets here, not required when no arguments are specified.

mawiesne · 2025-04-21T08:30:50Z

opennlp-tools/src/test/java/opennlp/tools/monitoring/LogLikelihoodThresholdBreachedTest.java

+        new TrainingParameters(Map.of(LOG_LIKELIHOOD_THRESHOLD_PARAM,5.)));
+  }
+
+  @ParameterizedTest()


Remove extra brackets here, not required when no arguments are specified.

mawiesne · 2025-04-21T08:31:13Z

pom.xml

@@ -180,6 +180,7 @@
 		<!-- Dependency versions -->
 		<junit.version>5.12.1</junit.version>
 		<junit5-system-exit.version>2.0.2</junit5-system-exit.version>
+		<assertj-core.version>3.27.3</assertj-core.version>


Avoid it, see comment(s) above.

NishantShri4 marked this pull request as draft March 24, 2025 07:28

rzo1 reviewed Mar 24, 2025

View reviewed changes

NishantShri4 commented Apr 14, 2025

View reviewed changes

NishantShri4 commented Apr 15, 2025

View reviewed changes

opennlp-tools/src/test/java/opennlp/tools/monitoring/DefaultTrainingProgressMonitorTest.java Show resolved Hide resolved

NishantShri4 marked this pull request as ready for review April 16, 2025 08:30

mawiesne changed the title ~~OPENNLP-124 Maxent/Perceptron training should report progess back via an API~~ OPENNLP-124: Maxent/Perceptron training should report progess back via an API Apr 16, 2025

NishantShri4 force-pushed the main branch from 9f8621e to d02cbaa Compare April 16, 2025 22:41

OPENNLP-124 : Maxent/Perceptron training should report progess back v…

c351fcd

…ia an API

NishantShri4 force-pushed the main branch from 7042b4a to c351fcd Compare April 16, 2025 22:54

Merge remote-tracking branch 'upstream/main'

f21d867

mawiesne requested changes Apr 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OPENNLP-124: Maxent/Perceptron training should report progess back via an API #758

OPENNLP-124: Maxent/Perceptron training should report progess back via an API #758

NishantShri4 commented Mar 24, 2025 •

edited

Loading

NishantShri4 commented Mar 24, 2025

mawiesne commented Mar 24, 2025

rzo1 left a comment

NishantShri4 commented Apr 13, 2025 •

edited

Loading

NishantShri4 Apr 14, 2025 •

edited

Loading

mawiesne Apr 21, 2025

NishantShri4 Apr 15, 2025

mawiesne Apr 21, 2025

NishantShri4 commented Apr 16, 2025

mawiesne commented Apr 16, 2025

NishantShri4 commented Apr 16, 2025

mawiesne left a comment

mawiesne Apr 21, 2025

mawiesne Apr 21, 2025

mawiesne Apr 21, 2025

mawiesne Apr 21, 2025

mawiesne Apr 21, 2025

mawiesne Apr 21, 2025

mawiesne Apr 21, 2025

mawiesne Apr 21, 2025

mawiesne Apr 21, 2025

mawiesne Apr 21, 2025

OPENNLP-124: Maxent/Perceptron training should report progess back via an API #758

Are you sure you want to change the base?

OPENNLP-124: Maxent/Perceptron training should report progess back via an API #758

Conversation

NishantShri4 commented Mar 24, 2025 • edited Loading

For all changes:

For code changes:

For documentation related changes:

Note:

NishantShri4 commented Mar 24, 2025

mawiesne commented Mar 24, 2025

rzo1 left a comment

Choose a reason for hiding this comment

NishantShri4 commented Apr 13, 2025 • edited Loading

NishantShri4 Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NishantShri4 commented Apr 16, 2025

mawiesne commented Apr 16, 2025

NishantShri4 commented Apr 16, 2025

mawiesne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NishantShri4 commented Mar 24, 2025 •

edited

Loading

NishantShri4 commented Apr 13, 2025 •

edited

Loading

NishantShri4 Apr 14, 2025 •

edited

Loading