[jira] [Commented] (OPENNLP-1448) Introduce SLF4J in OpenNLP

2023-02-09 Thread ASF GitHub Bot (Jira)


[ 
https://issues.apache.org/jira/browse/OPENNLP-1448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17686557#comment-17686557
 ] 

ASF GitHub Bot commented on OPENNLP-1448:
-

rzo1 commented on PR #492:
URL: https://github.com/apache/opennlp/pull/492#issuecomment-1424276095

   > Reviewed 63 out of 150 files smiling_face_with_tear Next time I have time 
I can either finish it or try running the code locally to verify it's working 
and its output. Thanks a lot!
   
   Yeah - no problem. Think we have time to do it steady & slowly ;-) - I will 
also re-iterate about the changes soon.




> Introduce SLF4J in OpenNLP
> --
>
> Key: OPENNLP-1448
> URL: https://issues.apache.org/jira/browse/OPENNLP-1448
> Project: OpenNLP
>  Issue Type: Sub-task
>Reporter: Richard Zowalla
>Assignee: Richard Zowalla
>Priority: Major
>
> This will be the first step regarding OPENNLP-1447.
> Goal is to replace System.err / System.out calls with logger output, which is 
> configurable.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)


[jira] [Commented] (OPENNLP-1448) Introduce SLF4J in OpenNLP

2023-02-09 Thread ASF GitHub Bot (Jira)


[ 
https://issues.apache.org/jira/browse/OPENNLP-1448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17686547#comment-17686547
 ] 

ASF GitHub Bot commented on OPENNLP-1448:
-

kinow commented on code in PR #492:
URL: https://github.com/apache/opennlp/pull/492#discussion_r1101145398


##
opennlp-distr/src/main/assembly/bin.xml:
##
@@ -48,7 +48,17 @@
755
.

-   
+
+   
+   src/main/resources
+   644
+   755
+   conf
+   
+   log4j2.xml
+   
+   
+

Review Comment:
   At least the portion of code I can see on GH UI is using spaces. But if this 
file has a mix of both spaces and tabs, no need to worry about this :+1: 



##
opennlp-distr/src/main/bin/opennlp:
##
@@ -37,4 +37,4 @@ fi
 # Might fail if $0 is a link
 OPENNLP_HOME=`dirname "$0"`/..
 
-$JAVACMD $HEAP -cp $(echo $OPENNLP_HOME/lib/*.jar | tr ' ' ':') 
opennlp.tools.cmdline.CLI $@
+$JAVACMD $HEAP -Dlog4j.configurationFile=../conf/log4j2.xml -cp $(echo 
$OPENNLP_HOME/lib/*.jar | tr ' ' ':') opennlp.tools.cmdline.CLI $@

Review Comment:
   Add missing newline?



##
opennlp-tools/src/main/java/opennlp/tools/log/LogPrintStream.java:
##
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+
+package opennlp.tools.log;
+
+import java.io.PrintStream;
+import java.util.Objects;
+
+import org.slf4j.Logger;
+import org.slf4j.event.Level;
+
+import opennlp.tools.commons.Internal;
+
+/**
+ * This class serves as an adapter for a {@link Logger} used withing a {@link 
PrintStream}.
+ */
+@Internal
+public class LogPrintStream extends PrintStream {
+
+  private final Logger logger;
+  private final Level level;
+
+  /**
+   * Creates a {@link LogPrintStream} for the given {@link Logger}
+   *
+   * @param logger must not be {@code null}
+   */
+  public LogPrintStream(Logger logger) {
+this(logger, Level.INFO);
+  }
+
+  /**
+   * Creates a {@link LogPrintStream} for the given {@link Logger}, which logs 
at the specified
+   * {@link Level level}.
+   *
+   * @param logger must not be {@code null}
+   * @param level  must not be {@code null}
+   */
+  public LogPrintStream(Logger logger, Level level) {
+super(nullOutputStream());
+Objects.requireNonNull(logger, "logger must not be NULL.");
+Objects.requireNonNull(level, "log level must not be NULL.");
+this.logger = logger;
+this.level = level;
+  }
+
+  @Override
+  public PrintStream printf(String format, Object... args) {
+log(String.format(format, args));
+return this;
+  }
+
+  @Override
+  public void println(String msg) {
+log(msg);
+  }
+
+  private void log(String msg) {
+switch (level) {
+  case TRACE:
+logger.trace(msg);
+  case DEBUG:
+logger.debug(msg);
+  case INFO:
+logger.info(msg);
+  case WARN:
+logger.warn(msg);
+  case ERROR:
+logger.error(msg);
+}

Review Comment:
   Isn't there a `logger.log(msg, level)` method in SLF4J? If so this method 
could be deleted, and the caller code adapted to call that instead? (I think I 
used a method like that, but can't recall the logging library, nor the 
programming language :rofl: )



##
opennlp-tools/src/main/java/opennlp/tools/ml/perceptron/SimplePerceptronSequenceTrainer.java:
##
@@ -372,14 +362,11 @@ public void nextIteration(int iteration) throws 
IOException {
   if (predParams[oi] != 0) {
 predParams[oi] /= totIterations;
 averageParams[pi].setParameter(oi, predParams[oi]);
-
//System.err.println("updates["+pi+"]["+oi+"]=("+updates[pi][oi][ITER]+","
-// +updates[pi][oi][EVENT]+","+updates[pi][oi][VALUE]+") + 
("+iterations+","+0+","
-// +params[pi].getParameters()[oi]+") -> 
"+averageParams[pi].getParameters()[oi]);
   }

Review Comment:
   These other output lines are not useful? Maybe at another log level?



##

[jira] [Comment Edited] (OPENNLP-1447) Move from System.out/System.err to SLF4J

2023-02-09 Thread Bruno P. Kinoshita (Jira)


[ 
https://issues.apache.org/jira/browse/OPENNLP-1447?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17686534#comment-17686534
 ] 

Bruno P. Kinoshita edited comment on OPENNLP-1447 at 2/9/23 2:06 PM:
-

For tests that rely on capturing the process output, maybe something like 
what's described on these SO answers could work? 
[https://stackoverflow.com/questions/29076981/how-to-intercept-slf4j-with-logback-logging-via-a-junit-test]

However, I think we still need to review if these tests have to be kept as they 
are (i.e. writing to some output stream) or if they should be re-written at an 
Java object input/output level, instead of I/O.


was (Author: kinow):
For tests that rely on capturing the process output, maybe something like 
what's described on these SO answers could work? 
https://stackoverflow.com/questions/29076981/how-to-intercept-slf4j-with-logback-logging-via-a-junit-test

> Move from System.out/System.err to SLF4J
> 
>
> Key: OPENNLP-1447
> URL: https://issues.apache.org/jira/browse/OPENNLP-1447
> Project: OpenNLP
>  Issue Type: Epic
>Reporter: Richard Zowalla
>Priority: Major
>
> As discussed on the mailing list, we are in favour of moving from System.out 
> / System.err to proper log output.
> The discussion is here: 
> https://lists.apache.org/thread/vt748qbz5onhwhh70kky9wk1o5zm42tm
> To reduce reviewer burden, we should tackle the task in several steps.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)


[jira] [Commented] (OPENNLP-1447) Move from System.out/System.err to SLF4J

2023-02-09 Thread Bruno P. Kinoshita (Jira)


[ 
https://issues.apache.org/jira/browse/OPENNLP-1447?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17686534#comment-17686534
 ] 

Bruno P. Kinoshita commented on OPENNLP-1447:
-

For tests that rely on capturing the process output, maybe something like 
what's described on these SO answers could work? 
https://stackoverflow.com/questions/29076981/how-to-intercept-slf4j-with-logback-logging-via-a-junit-test

> Move from System.out/System.err to SLF4J
> 
>
> Key: OPENNLP-1447
> URL: https://issues.apache.org/jira/browse/OPENNLP-1447
> Project: OpenNLP
>  Issue Type: Epic
>Reporter: Richard Zowalla
>Priority: Major
>
> As discussed on the mailing list, we are in favour of moving from System.out 
> / System.err to proper log output.
> The discussion is here: 
> https://lists.apache.org/thread/vt748qbz5onhwhh70kky9wk1o5zm42tm
> To reduce reviewer burden, we should tackle the task in several steps.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)