brainglobe / cellfinder

Automated 3D cell detection in very large images
https://brainglobe.info/documentation/cellfinder/index.html
BSD 3-Clause "New" or "Revised" License
176 stars 39 forks source link

Tests are failing with napari 0.5.0 #443

Open adamltyson opened 1 month ago

adamltyson commented 1 month ago

@alessandrofelder's hypothesis - "napari fixtures are now cleverer and fail your tests if you don't clean up your widgets properly"

We should look into this and get the tests passing again (e.g. by making sure all qt widgets have parents)

IgorTatarnikov commented 1 month ago

Based on my investigations I think this is related to the logging level set, if "DEBUG" is set then these errors crop up while if everything is "INFO" and below it seems to work fine. Upon further investigation this seems to only matter for logging to file and not to console.

This was the minimal example I've been working with:

import numpy as np
from fancylog import fancylog
from pathlib import Path
import cellfinder.core as program_for_log

def test_example(make_napari_viewer):
    package_path = Path(__file__).parent

    fancylog.start_logging(
        package_path,
        program_for_log,
        file_log_level="INFO",
        log_header="CELLFINDER TRAINING LOG",
    )

    viewer = make_napari_viewer()
    test_image = np.random.random((100, 100, 100))
    n_layers = len(viewer.layers)
    # adds a "detected" and a "rejected layer"
    viewer.add_image(test_image, name="test_image")

    assert len(viewer.layers) == n_layers + 1

From what I understand it has something to do with the use of WeakSet by napari during clean up. Logging to file with DEBUG keeps a reference to the viewer open and that breaks the teardown code.

IgorTatarnikov commented 1 month ago

I've got a better minimal example now that doesn't use fancylog I think this needs to be passed to the napari developers. I've verified that 3 out of 4 tests pass using napari==0.5.0 and 4 for 4 using napari==0.4.19.post1

import numpy as np
import logging

import pytest

@pytest.mark.parametrize("debug_level", ["INFO", "WARNING", "ERROR", "DEBUG",])
def test_example(make_napari_viewer, debug_level):
    logger = logging.getLogger()
    logger.setLevel(getattr(logging, debug_level))

    viewer = make_napari_viewer()
    test_image = np.random.random((100, 100, 100))
    n_layers = len(viewer.layers)
    # adds a "detected" and a "rejected layer"
    viewer.add_image(test_image, name="test_image")

    assert len(viewer.layers) == n_layers + 1
adamltyson commented 1 month ago

Thanks @IgorTatarnikov, yes I'd say it's worth raising an issue, or asking on Zulip.

My only confusion though - I don't think the cellfinder plugin (unlike brainreg) logs to file?

IgorTatarnikov commented 1 month ago

The cellfinder plugin doesn't directly, there's only one time logging is invoked during our test suite:

https://github.com/brainglobe/cellfinder/blob/71d17eee54951edf2d2e9f0bd7ead4244a0f6bae/tests/core/test_integration/test_train.py#L36

Which calls:

https://github.com/brainglobe/cellfinder/blob/71d17eee54951edf2d2e9f0bd7ead4244a0f6bae/cellfinder/core/train/train_yml.py#L265-L274

Commenting out that one line fixes the errors.

alessandrofelder commented 1 month ago

The cellfinder plugin doesn't directly, there's only one time logging is invoked during our test suite:

So the logger from a non-napari-related test function persists across tests??? :scream: TIL!

adamltyson commented 1 month ago

Just noting that following #444 we should make sure to update the tests (removing the xfail) when this issue is addressed.