Fix issue in utils.progress for disable=True #5964

constantinpape · 2023-06-19T08:26:31Z

Fixes/Closes

Description and references

This PR changes the progress.__init__ such that if the progress bar is disabled, the 'desc' property is set to a dummy value, because otherwise it is not set by the tqdm super constructor and throws an error.

Type of change

Bug-fix (non-breaking change which fixes an issue)

How has this been tested?

I checked locally that this test fixes this code:

# from tqdm import tqdm
from napari.utils import progress as tqdm

for z in tqdm(range(10), total=10, desc="Count to 10", disable=True):
    pass

which throws an AttributeError without the fix.

Final checklist:

My PR is the minimum possible work for the desired functionality
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
If I included new strings, I have used trans. to make them localizable.
For more information see our translations guide.

Fixes napari#5961

constantinpape · 2023-06-19T08:30:21Z

@DragaDoncila here's the PR for #5961

codecov · 2023-06-19T09:24:10Z

Codecov Report

Merging #5964 (2ac83e0) into main (efa55a5) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5964      +/-   ##
==========================================
- Coverage   90.19%   90.19%   -0.01%     
==========================================
  Files         615      615              
  Lines       52039    52045       +6     
==========================================
+ Hits        46939    46942       +3     
- Misses       5100     5103       +3

Impacted Files	Coverage Δ
napari/utils/_tests/test_progress.py	`100.00% <100.00%> (ø)`
napari/utils/progress.py	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

Czaki · 2023-06-19T09:36:50Z

Could you add test? It looks like a nice thing to backport to 0.4.18 but I will appreciate tests for any such thing to not need manually test it.

constantinpape · 2023-06-19T10:47:40Z

Are there any tests for the progressbar already where I could just add this one, @Czaki ?

psobolewskiPhD · 2023-06-19T10:55:13Z

@constantinpape looks like here would work?
https://github.com/napari/napari/blob/main/napari/utils/_tests/test_progress.py
Maybe after/around:

napari/napari/utils/_tests/test_progress.py

Line 80 in efa55a5

def test_progress_set_description():

Also, before this is merged, could you just ensure the PR description is self-sufficient, rather than the See Issue...?
The PR description will be the commit message, so we'd like titles and descriptions to be descriptive, grammatically correct, and thorough, so we have a nice commit history and nice release notes.

for more information, see https://pre-commit.ci

constantinpape · 2023-06-19T11:23:21Z

I have added a test now and updated the PR description.

psobolewskiPhD · 2023-06-19T13:41:29Z

Maybe the reproducer and error (which are in the issue) could be replaced with something like:

In this PR a change is made to the progress.__init__ such that if the progress bar is set to be disabled, the 'desc' property set it to a dummy value, because otherwise it is not set by the tqdm super constructor and throws an error.

(if i'm getting the nature of the change correctly from the code and comment)

Czaki · 2023-06-19T13:43:23Z

@constantinpape There is a need to update the test as they are failing.

napari/utils/_tests/test_progress.py

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>

napari/utils/_tests/test_progress.py

constantinpape · 2023-06-19T14:22:03Z

Thanks for fixing the tests @psobolewskiPhD. This is good to go from my side now.

Czaki · 2023-06-19T14:33:40Z

@constantinpape as it is your first contribution to napari, and it will be backported to 0.4.18. Could you provide data to be put in https://github.com/napari/napari/blob/main/CITATION.cff ?

constantinpape · 2023-06-19T14:45:28Z

I have added the citation info in 2ac73e0.

constantinpape · 2023-06-19T17:08:52Z

PR Test / ubuntu-latest 3.9 pyside6 (pull_request) is timing out. This seems to be unrelated to the changes here.

DragaDoncila

Thank you for the contribution @constantinpape! 🎉

# Fixes/Closes Closes #5961 # Description and references This PR changes the `progress.__init__` such that if the progress bar is disabled, the 'desc' property is set to a dummy value, because otherwise it is not set by the tqdm super constructor and throws an error. ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # How has this been tested? A test `test_progress_set_disable` has been added to napari/utils/_tests/test_progress.py I checked locally that this test fixes this code: ```python # from tqdm import tqdm from napari.utils import progress as tqdm for z in tqdm(range(10), total=10, desc="Count to 10", disable=True): pass ``` which throws an `AttributeError` without the fix. ## Final checklist: - [x] My PR is the minimum possible work for the desired functionality - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added tests that prove my fix is effective or that my feature works --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>

# Fixes/Closes Closes napari#5961 # Description and references This PR changes the `progress.__init__` such that if the progress bar is disabled, the 'desc' property is set to a dummy value, because otherwise it is not set by the tqdm super constructor and throws an error. ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # How has this been tested? A test `test_progress_set_disable` has been added to napari/utils/_tests/test_progress.py I checked locally that this test fixes this code: ```python # from tqdm import tqdm from napari.utils import progress as tqdm for z in tqdm(range(10), total=10, desc="Count to 10", disable=True): pass ``` which throws an `AttributeError` without the fix. ## Final checklist: - [x] My PR is the minimum possible work for the desired functionality - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added tests that prove my fix is effective or that my feature works --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>

Fix issue in utils.progress for disable=True

45a5c43

Fixes napari#5961

github-actions bot assigned constantinpape Jun 19, 2023

psobolewskiPhD requested a review from DragaDoncila June 19, 2023 09:04

psobolewskiPhD added the bugfix PR with bugfix label Jun 19, 2023

Czaki added this to the 0.4.18 milestone Jun 19, 2023

Add test for disabling the progress bar

dd3ee51

github-actions bot added the tests Something related to our tests label Jun 19, 2023

[pre-commit.ci] auto fixes from pre-commit.com hooks

bd85b6c

for more information, see https://pre-commit.ci

Czaki approved these changes Jun 19, 2023

View reviewed changes

Czaki requested a review from a team June 19, 2023 11:21

psobolewskiPhD reviewed Jun 19, 2023

View reviewed changes

napari/utils/_tests/test_progress.py Outdated Show resolved Hide resolved

Update napari/utils/_tests/test_progress.py

516d307

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>

psobolewskiPhD reviewed Jun 19, 2023

View reviewed changes

napari/utils/_tests/test_progress.py Outdated Show resolved Hide resolved

fix formatting

6adee75

psobolewskiPhD approved these changes Jun 19, 2023

View reviewed changes

psobolewskiPhD added the ready to merge Last chance for comments! Will be merged in ~24h label Jun 19, 2023

Add citation info

2ac83e0

Czaki mentioned this pull request Jun 19, 2023

v0.4.18 - release process #5911

Closed

DragaDoncila approved these changes Jun 20, 2023

View reviewed changes

GenevieveBuckley merged commit a0fa476 into napari:main Jun 20, 2023
35 checks passed

psobolewskiPhD removed the ready to merge Last chance for comments! Will be merged in ~24h label Jun 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue in utils.progress for disable=True #5964

Fix issue in utils.progress for disable=True #5964

constantinpape commented Jun 19, 2023 •

edited

Loading

constantinpape commented Jun 19, 2023

codecov bot commented Jun 19, 2023 •

edited

Loading

Czaki commented Jun 19, 2023

constantinpape commented Jun 19, 2023

psobolewskiPhD commented Jun 19, 2023

constantinpape commented Jun 19, 2023

psobolewskiPhD commented Jun 19, 2023

Czaki commented Jun 19, 2023

constantinpape commented Jun 19, 2023

Czaki commented Jun 19, 2023

constantinpape commented Jun 19, 2023

constantinpape commented Jun 19, 2023

DragaDoncila left a comment

Fix issue in utils.progress for disable=True #5964

Fix issue in utils.progress for disable=True #5964

Conversation

constantinpape commented Jun 19, 2023 • edited Loading

Fixes/Closes

Description and references

Type of change

How has this been tested?

Final checklist:

constantinpape commented Jun 19, 2023

codecov bot commented Jun 19, 2023 • edited Loading

Codecov Report

Czaki commented Jun 19, 2023

constantinpape commented Jun 19, 2023

psobolewskiPhD commented Jun 19, 2023

constantinpape commented Jun 19, 2023

psobolewskiPhD commented Jun 19, 2023

Czaki commented Jun 19, 2023

constantinpape commented Jun 19, 2023

Czaki commented Jun 19, 2023

constantinpape commented Jun 19, 2023

constantinpape commented Jun 19, 2023

DragaDoncila left a comment

Choose a reason for hiding this comment

constantinpape commented Jun 19, 2023 •

edited

Loading

codecov bot commented Jun 19, 2023 •

edited

Loading