samtools / htsjdk

A Java API for high-throughput sequencing data (HTS) formats.
http://samtools.github.io/htsjdk/
283 stars 242 forks source link

changing WARNING to WARN and deprecating WARN #1270

Closed lbergelson closed 5 years ago

lbergelson commented 5 years ago

@heuermh Does this work for what you need?

codecov-io commented 5 years ago

Codecov Report

Merging #1270 into master will increase coverage by 0.007%. The diff coverage is 92.308%.

@@              Coverage Diff               @@
##             master     #1270       +/-   ##
==============================================
+ Coverage     69.36%   69.367%   +0.007%     
  Complexity     8306      8306               
==============================================
  Files           555       555               
  Lines         33130     33138        +8     
  Branches       5575      5575               
==============================================
+ Hits          22979     22987        +8     
  Misses         7887      7887               
  Partials       2264      2264
Impacted Files Coverage Δ Complexity Δ
src/main/java/htsjdk/samtools/util/Log.java 63.768% <92.308%> (+4.752%) 17 <3> (ø) :arrow_down:
yfarjoun commented 5 years ago

@heuermh please let us know if this works for you so we can use it instead of your PR #1262

heuermh commented 5 years ago

Sorry, I won't be able to test this downstream (without cherry-picking to an old release tag) until other version conflicts are resolved, e.g. https://github.com/disq-bio/disq/pull/84

lbergelson commented 5 years ago

@heuermh I don't understand the motivation for this change. Why is it important that this matches slf4j names? Is there some sort of filtering based on the names?

lbergelson commented 5 years ago

I'm going to close this one until I understand the reasoning better. I'm happy to do this if it's a significant problem that has functional issues with slf4, but if it's just that the names are inconsistent I don't think it's worth changing.

heuermh commented 5 years ago

Please consider keeping this open until we resolve the version conflicts downstream, I can't test the changes otherwise.

I don't understand the motivation for this change. Why is it important that this matches slf4j names? Is there some sort of filtering based on the names?

I can't say exactly how htsjdk isn't playing nice, the result is that even with https://github.com/disq-bio/disq/pull/80 we can't configure htsjdk logging to be silent.

disq master
$ mvn clean install
[INFO] Scanning for projects...
[INFO]
[INFO] -------------------------< org.disq-bio:disq >--------------------------
[INFO] Building Disq 0.2.1-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
...
[INFO] --- maven-surefire-plugin:3.0.0-M1:test (default-test) @ disq ---
[INFO]
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
...
[INFO] Running org.disq_bio.disq.TbiMergingTest
WARNING 2019-02-25 13:52:24 AsciiLineReader Creating an indexable source for an AsciiFeatureCodec using a stream that is neither a PositionalBufferedStream nor a BlockCompressedInputStream
WARNING 2019-02-25 13:52:24 AsciiLineReader Creating an indexable source for an AsciiFeatureCodec using a stream that is neither a PositionalBufferedStream nor a BlockCompressedInputStream
...
lbergelson commented 5 years ago

@heuermh Have you tried setting Log.setGlobalLogLevel(ERROR) which will raise it to only emit ERROR level warnings?

Does slf4j simple filter things based on the name of the warning message? I'm trying to understand why changing the name will make a difference.

heuermh commented 5 years ago

Have you tried setting Log.setGlobalLogLevel(ERROR) which will raise it to only emit ERROR level warnings?

All logging configuration should be in one place, the slf4j configuration specifically. We don't want to have to make calls in code in Disq. I believe htsjdk Log goes to java.util.logging which is then adapted to slf4j via org.slf4j:jul-to-slf4j, and there is some mismatch in there between WARN and WARNING, which my earlier pull request and presumably this one corrects.

The real fix is to drop the htsjdk Log shim for the slf4j API and let downstream users specify the logging implementation, but I can see how that is more of an effort.

The Library section of the SLF4J Manual goes through the rationale.

lbergelson commented 5 years ago

@heuermh I think I see the problem. Your expectations for this logger are too high! Htsjk's Log doesn't use java.util.Logging. It writes directly to stderr or whatever stream you specify, so jul-to-slf4j won't do anything to it.

I don't think the WARN/WARNING problem is the issue. I haven't looked into it, but I wonder if it would be easy to write an htsjdk-to-slf4j adapter. It has methods already to allow you to change the output stream arbitrarily as well as to programmatically change the log level.