brainglobe / brainrender-napari

A napari plugin to render BrainGlobe atlases and associated data as layers.
BSD 3-Clause "New" or "Revised" License
18 stars 1 forks source link

offload atlas downloading to separate thread #63

Closed alessandrofelder closed 10 months ago

alessandrofelder commented 11 months ago

Description

What is this PR

Why is this PR needed? Downloading an atlas should not block the napari viewer

What does this PR do? Offloads the atlas downloading to a separate thread.

References

Closes #7

How has this PR been tested?

Existing unit test adapted to wait for worker to finish.

Is this a breaking change?

Nope.

Does this PR require an update to the documentation?

n/a

Checklist:

alessandrofelder commented 10 months ago

Needs to be rebased onto #67

willGraham01 commented 10 months ago

Not sure what the rebase has done, but this looks like it's a bug in the unit test itself rather than in the implementation.

I can pip install -e . this branch, and quite happily be downloading a new atlas whilst looking at an existing one (see below). This is in a clean mamba environment with python 3.8 (which is the failing case).

image

Updates:

alessandrofelder commented 10 months ago

Argh! I know what's likely happening: my assumption didn't hold on the CI... (and this then breaks the rest of the tests :cry: )

I'll have to do a small refactor

willGraham01 commented 10 months ago

Argh! I know what's likely happening: my assumption didn't hold on the CI... (and this then breaks the rest of the tests 😢 )

I'm able to get the tests to pass w/ Python 3.8, 3.9, and 3.10 via tox locally, so that provides extra evidence for your claim.

codecov[bot] commented 10 months ago

Codecov Report

Patch coverage: 85.71% and project coverage change: -0.61% :warning:

Comparison is base (adf5ce6) 97.32% compared to head (3590a0e) 96.72%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #63 +/- ## ========================================== - Coverage 97.32% 96.72% -0.61% ========================================== Files 7 7 Lines 299 305 +6 ========================================== + Hits 291 295 +4 - Misses 8 10 +2 ``` | [Files Changed](https://app.codecov.io/gh/brainglobe/brainrender-napari/pull/63?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=brainglobe) | Coverage Δ | | |---|---|---| | [brainrender\_napari/widgets/atlas\_table\_view.py](https://app.codecov.io/gh/brainglobe/brainrender-napari/pull/63?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=brainglobe#diff-YnJhaW5yZW5kZXJfbmFwYXJpL3dpZGdldHMvYXRsYXNfdGFibGVfdmlldy5weQ==) | `97.89% <85.71%> (-2.11%)` | :arrow_down: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alessandrofelder commented 10 months ago

The line that codeCov is unhappy about is actually covered implicitly, because the test checks the the argument of the emitted signal, which is the return value of the thread worker