Replace loci_tools.jar with bioformats_package.jar #403

GenevieveBuckley · 2022-03-18T07:32:34Z

Closes #400

This PR aims to do two things:

Replace loci_tools.jar with bioformats_package.jar, and
Replace log4j with logback

We want to do this due to the Log4Shell vulnerability (for more details, see Josh's post here https://forum.image.sc/t/cve-2021-44228-log4shell-assessment-for-omero-bio-formats/61032 and the linked github issues above)

GenevieveBuckley · 2022-03-18T07:38:32Z

Current status:

Done: I have replaced loci_tools.jar with bioformats_package.jar

Not working: I still need to figure out how to add and use logback. I think my main problem is just that I don't know where the logback files live. @joshmoore do you have any advice for me? I think it's just these four lines we need to change:

pims/pims/bioformats.py

Lines 347 to 350 in 68c560e

    
           log4j = jpype.JPackage('org.apache.log4j') 
        
           log4j.BasicConfigurator.configure() 
        
           log4j_logger = log4j.Logger.getRootLogger() 
        
           log4j_logger.setLevel(log4j.Level.ERROR)

Unrelated to this PR, but there are a number of failing tests observed with pytest pims/tests/test_common.py. I also observe this on the main branch. I will open a separate issue for this problem.

joshmoore · 2022-03-21T14:50:07Z

Hi @GenevieveBuckley. Sorry, I just saw this. Are things now working for you with 43c9b7f ?

GenevieveBuckley · 2022-03-22T04:43:11Z

Not quite. I think I've inched a little bit closer.

Changed

I've changed these lines

pims/pims/bioformats.py

Lines 347 to 348 in 68c560e

    
           log4j = jpype.JPackage('org.apache.log4j') 
        
           log4j.BasicConfigurator.configure()

to this instead

logback = jpype.JPackage('ch.qos.logback.classic')
logger_context = logback.LoggerContext() logback.BasicConfigurator().setContext(logger_context)
logback.BasicConfigurator().configure(logger_context)

I'm relatively confident that's probably right, but I don't know how to test it for sure.

Still stuck on this

I'm still stuck on what these two lines should be:

pims/pims/bioformats.py

Lines 349 to 350 in 68c560e

    
           log4j_logger = log4j.Logger.getRootLogger() 
        
           log4j_logger.setLevel(log4j.Level.ERROR)

I can figure out how to get logback.Level.ERROR, but I couldn't work out how to get the root logger. So that's where things are at.

joshmoore · 2022-03-22T14:24:16Z

Gotcha. It might be easier to use the DebugToolsfrom https://docs.openmicroscopy.org/bio-formats/6/developers/logging.html where you can pass a string:

DebugTools.setRootLevel("OFF")

This prevents the need to load more of the logback internals. But if that is necessary, you can see all the necessary classes & code in https://github.com/ome/ome-common-java/blob/9685c5d2f6a7d05ee6a100987fe34c0d573f54a5/src/main/java/loci/common/LogbackTools.java#L75-L84

  /**
   * Sets the level of the root logger
   *
   * @param level A string indicating the desired level
   *   (i.e.: ALL, DEBUG, ERROR, FATAL, INFO, OFF, WARN).
   */
  public static synchronized void setRootLevel(String level) {
    Logger root = (Logger) LoggerFactory.getLogger(Logger.ROOT_LOGGER_NAME);
    root.setLevel(Level.toLevel(level));
  }

Note: DebugTools in that same directory just wraps LogbackTools & Log4jTools to prevent the user from needing to know.

tacaswell · 2022-04-07T20:32:17Z

@GenevieveBuckley Sorry, I made this need a rebase via #399

tacaswell · 2022-04-07T20:49:03Z

I took the liberty of rebasing, hopefully that is OK.

tacaswell · 2022-04-12T16:15:21Z

I rebased again.

What is the state of this? I would like to do a release ASAP and getting in log4j fixes is obviously good!

joshmoore · 2022-04-12T16:21:50Z

Deferring to @GenevieveBuckley on the functionality she wanted, but reading the code 👍

GenevieveBuckley · 2022-04-26T03:59:41Z

Status: log4j is replaced by logback, but it may not log things correctly. Current status is summarized here #403 (comment)

I haven't had time to try Josh's suggestion here. It is unlikely I can prioritize that on the timeframe you want. Do others have more bandwidth right now?

tacaswell · 2022-04-26T18:04:57Z

OK, I'm going to do the release without this and trust that someone down-stream is making sure that patched versions of log4j are being used.

https://docs.openmicroscopy.org/bio-formats/6.9.1/developers/logging.html Approach suggested by Josh Moore

codecov-commenter · 2022-04-27T07:29:46Z

Codecov Report

Merging #403 (9f0cccc) into main (ab00b0a) will decrease coverage by 0.03%.
The diff coverage is 8.33%.

@@            Coverage Diff             @@
##             main     #403      +/-   ##
==========================================
- Coverage   62.61%   62.58%   -0.04%     
==========================================
  Files          31       31              
  Lines        4486     4498      +12     
==========================================
+ Hits         2809     2815       +6     
- Misses       1677     1683       +6

Impacted Files	Coverage Δ
pims/tests/test_bioformats.py	`39.25% <0.00%> (ø)`
pims/bioformats.py	`14.33% <8.69%> (+0.19%)`	⬆️
pims/base_frames.py	`76.97% <0.00%> (-0.21%)`	⬇️
pims/tiff_stack.py	`57.67% <0.00%> (-0.11%)`	⬇️
pims/tests/test_common.py	`88.29% <0.00%> (-0.03%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab00b0a...9f0cccc. Read the comment docs.

GenevieveBuckley · 2022-04-27T23:12:28Z

Update: I've added the line loci.common.DebugTools.enableLogging('ERROR').

I think Josh is right, the DebugTools logging is perhaps the easiest way. The other suggestion is not possible on my machine for the same reason I had trouble earlier (anytime I try to access Logger, I get a class definition error).

I swear at one point all the tests passed with pytest pims/tests/test_bioformats.py, but then I changed something about my environment and that's no longer the case. I've also regenerated a development environment, and still see failing tests. There's a variety of reasons: some deprecation warnings in libraries like slicerator, something about the norpix detector in pims not recognising a file format. I haven't teased apart what is actually relevant to this PR yet.

tacaswell · 2022-04-28T04:26:21Z

There's a variety of reasons: some deprecation warnings in libraries like slicerator,

We did a slicerator release so I thought at least that one should be fixed...

GenevieveBuckley · 2022-05-13T03:20:22Z

More details on the failing tests here: #416
I think those problems are separate to this PR

jstriebel · 2022-11-14T11:11:03Z

Any updates here? This would be nice to have, also to be able to use the newest bioformats versions, which do not include the loci_tools.jar anymore.

joshmoore · 2022-11-15T11:51:48Z

All the changes here are still 👍 from the OME point-of-view.

GenevieveBuckley · 2022-11-22T03:03:35Z

No, no change here.
I'm not blocking it, I think it just got stuck in limbo because if no-one could confirm whether the test suite passes, then no-one could be sure it wouldn't introduce a new problem.

GenevieveBuckley · 2022-11-22T03:18:17Z

...and that's still the case today.
I've just made a new pims-dev conda environment, and am unable to successfully run the test suite on the main branch.

====================================== short test summary info =======================================
FAILED pims/tests/test_display.py::TestPlotToFrame::test_ax_to_frame - PendingDeprecationWarning: The set_tight_layout function will be deprecated in a future version. ...
FAILED pims/tests/test_display.py::TestPlotToFrame::test_axes_to_frame - PendingDeprecationWarning: The set_tight_layout function will be deprecated in a future version. ...
FAILED pims/tests/test_display.py::TestPlotToFrame::test_plot_resize - PendingDeprecationWarning: The set_tight_layout function will be deprecated in a future version. ...
FAILED pims/tests/test_display.py::TestPlotToFrame::test_plot_tight - PendingDeprecationWarning: The set_tight_layout function will be deprecated in a future version. ...
FAILED pims/tests/test_display.py::TestPlotToFrame::test_plot_to_frame - PendingDeprecationWarning: The set_tight_layout function will be deprecated in a future version. ...
FAILED pims/tests/test_display.py::TestPlotToFrame::test_plot_width - PendingDeprecationWarning: The set_tight_layout function will be deprecated in a future version. ...
FAILED pims/tests/test_display.py::TestPlotToFrame::test_plots_from_generator - PendingDeprecationWarning: The set_tight_layout function will be deprecated in a future version. ...
FAILED pims/tests/test_display.py::TestPlotToFrame::test_plots_resize - PendingDeprecationWarning: The set_tight_layout function will be deprecated in a future version. ...
FAILED pims/tests/test_display.py::TestPlotToFrame::test_plots_tight - PendingDeprecationWarning: The set_tight_layout function will be deprecated in a future version. ...
FAILED pims/tests/test_display.py::TestPlotToFrame::test_plots_to_frame - PendingDeprecationWarning: The set_tight_layout function will be deprecated in a future version. ...
FAILED pims/tests/test_display.py::TestPlotToFrame::test_plots_width - PendingDeprecationWarning: The set_tight_layout function will be deprecated in a future version. ...
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_as_grey - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_bool - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_composed_pipelines - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_count - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_filename_tzc - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_frame_number_accurate - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_frame_number_present - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_getting_list - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_getting_single_frame - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_getting_slice - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_integer_attributes - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_iterator - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_pipeline_simple - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_pipeline_with_args - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_repr - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_shape - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_simple_negative_index - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_sizeC - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_sizeZ - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND::test_slice_of_slice - ValueError: axis 'c' already exists
FAILED pims/tests/test_imseq.py::ImageSequenceND_RGB::test_as_grey - IndexError: index 0 is out of bounds for axis 0 with size 0
FAILED pims/tests/test_imseq.py::ImageSequenceND_RGB::test_composed_pipelines - IndexError: index 0 is out of bounds for axis 0 with size 0
FAILED pims/tests/test_imseq.py::ImageSequenceND_RGB::test_count - AssertionError:
FAILED pims/tests/test_imseq.py::ImageSequenceND_RGB::test_frame_number_accurate - IndexError: index 0 is out of bounds for axis 0 with size 0
FAILED pims/tests/test_imseq.py::ImageSequenceND_RGB::test_frame_number_present - IndexError: index 0 is out of bounds for axis 0 with size 0
FAILED pims/tests/test_imseq.py::ImageSequenceND_RGB::test_getting_list - IndexError: index 0 is out of bounds for axis 0 with size 0
FAILED pims/tests/test_imseq.py::ImageSequenceND_RGB::test_getting_single_frame - IndexError: index 0 is out of bounds for axis 0 with size 0
FAILED pims/tests/test_imseq.py::ImageSequenceND_RGB::test_getting_slice - IndexError: index 0 is out of bounds for axis 0 with size 0
FAILED pims/tests/test_imseq.py::ImageSequenceND_RGB::test_iterator - IndexError: index 0 is out of bounds for axis 0 with size 0
FAILED pims/tests/test_imseq.py::ImageSequenceND_RGB::test_pipeline_simple - IndexError: index 0 is out of bounds for axis 0 with size 0
FAILED pims/tests/test_imseq.py::ImageSequenceND_RGB::test_pipeline_with_args - IndexError: index 0 is out of bounds for axis 0 with size 0
FAILED pims/tests/test_imseq.py::ImageSequenceND_RGB::test_shape - AssertionError:
FAILED pims/tests/test_imseq.py::ImageSequenceND_RGB::test_simple_negative_index - IndexError: index 0 is out of bounds for axis 0 with size 0
FAILED pims/tests/test_imseq.py::ImageSequenceND_RGB::test_sizeZ - AssertionError:
FAILED pims/tests/test_imseq.py::ImageSequenceND_RGB::test_slice_of_slice - IndexError: index 0 is out of bounds for axis 0 with size 0
FAILED pims/tests/test_imseq.py::ReaderSequence::test_count - AssertionError:
FAILED pims/tests/test_imseq.py::ReaderSequence::test_simple_negative_index - RuntimeError: /Users/genevieb/Documents/GitHub/pims/pims/tests/data/image_sequence3d/file_t003_z0...

If the main branch tests aren't passing, that means I don't have a good way to test the changes on this PR branch. So the status is the same: I think my changes here are helpful, but I have no way to know for sure.

This is probably something that a maintainer is going to need to dig into. There are a handful of open issues about failing test on main:

GenevieveBuckley mentioned this pull request Mar 18, 2022

Failing tests on main branch #404

Closed

tacaswell force-pushed the bioformats-package-jar branch from 43c9b7f to 1075f48 Compare April 7, 2022 20:48

GenevieveBuckley added 4 commits April 12, 2022 12:13

Replace references to log4j with logback

9ea8fcb

Pull latest version of bioformats_package.jar file by default

1598c8a

Update bioformats documentation to avoid ImportError

1e48398

Temporarily remove non-functional java logger code

493295f

tacaswell force-pushed the bioformats-package-jar branch from 1075f48 to fb0b988 Compare April 12, 2022 16:14

Java logback configuration

fb0b988

nkeim added this to the v0.6 milestone Apr 20, 2022

nkeim mentioned this pull request Apr 20, 2022

RLS: Release notes for v0.6 #412

Merged

4 tasks

tacaswell modified the milestones: v0.6, v0.7 Apr 26, 2022

tacaswell mentioned this pull request Apr 26, 2022

Add complete list of supported bioformats filetypes #357

Open

Set root log level with loci DebugTools

0f4c69e

https://docs.openmicroscopy.org/bio-formats/6.9.1/developers/logging.html Approach suggested by Josh Moore

GenevieveBuckley added 2 commits May 12, 2022 14:36

Replace deprecated distutils LooseVersion with packaging Version

662aa70

Update unittest syntaxto more modern version

9f0cccc

GenevieveBuckley mentioned this pull request May 13, 2022

Failing tests in test_bioformats.py #416

Closed

mwakok mentioned this pull request May 25, 2022

Java complications when loading lifs koenderinklab/OpenDDM#15

Open

GenevieveBuckley mentioned this pull request Nov 22, 2022

loci_tools.jar issue #426

Open

joshmoore mentioned this pull request Mar 17, 2023

Vulnerabilities still unfixed #443

Open

joshmoore mentioned this pull request Jan 11, 2024

loci_tools.jar missing #456

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace loci_tools.jar with bioformats_package.jar #403

Replace loci_tools.jar with bioformats_package.jar #403

GenevieveBuckley commented Mar 18, 2022

GenevieveBuckley commented Mar 18, 2022 •

edited

Loading

joshmoore commented Mar 21, 2022

GenevieveBuckley commented Mar 22, 2022

joshmoore commented Mar 22, 2022

tacaswell commented Apr 7, 2022

tacaswell commented Apr 7, 2022

tacaswell commented Apr 12, 2022

joshmoore commented Apr 12, 2022

GenevieveBuckley commented Apr 26, 2022

tacaswell commented Apr 26, 2022

codecov-commenter commented Apr 27, 2022 •

edited

Loading

GenevieveBuckley commented Apr 27, 2022

tacaswell commented Apr 28, 2022

GenevieveBuckley commented May 13, 2022

jstriebel commented Nov 14, 2022

joshmoore commented Nov 15, 2022

GenevieveBuckley commented Nov 22, 2022

GenevieveBuckley commented Nov 22, 2022

Replace loci_tools.jar with bioformats_package.jar #403

Are you sure you want to change the base?

Replace loci_tools.jar with bioformats_package.jar #403

Conversation

GenevieveBuckley commented Mar 18, 2022

GenevieveBuckley commented Mar 18, 2022 • edited Loading

joshmoore commented Mar 21, 2022

GenevieveBuckley commented Mar 22, 2022

Changed

Still stuck on this

joshmoore commented Mar 22, 2022

tacaswell commented Apr 7, 2022

tacaswell commented Apr 7, 2022

tacaswell commented Apr 12, 2022

joshmoore commented Apr 12, 2022

GenevieveBuckley commented Apr 26, 2022

tacaswell commented Apr 26, 2022

codecov-commenter commented Apr 27, 2022 • edited Loading

Codecov Report

GenevieveBuckley commented Apr 27, 2022

tacaswell commented Apr 28, 2022

GenevieveBuckley commented May 13, 2022

jstriebel commented Nov 14, 2022

joshmoore commented Nov 15, 2022

GenevieveBuckley commented Nov 22, 2022

GenevieveBuckley commented Nov 22, 2022

GenevieveBuckley commented Mar 18, 2022 •

edited

Loading

codecov-commenter commented Apr 27, 2022 •

edited

Loading