Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: globally disable progress bars #5207

Merged
merged 4 commits into from
Jun 27, 2023
Merged

feat: globally disable progress bars #5207

merged 4 commits into from
Jun 27, 2023

Conversation

ZanSara
Copy link
Contributor

@ZanSara ZanSara commented Jun 26, 2023

Related Issues

Proposed Changes:

  • Add SilenceableTqdm, a class that wraps tqdm and automatically disables progress bars if HAYSTACK_PROGRESS_BARS is set to some falsey value (0, False, FALSE, false).
  • Change all imports to from tqdm import tqdm, as the from tqdm.auto import tqdm was found really hard to intercept and wrap correctly.

For @dfokina: to disable all Haystack progress bars, users can now set and environment variable called HAYSTACK_PROGRESS_BARS to one of 0, False, FALSE, false. For example, in Bash (Linux) it's like export HAYSTACK_PROGRESS_BARS=0. That's enough to disable all progress bars.

How did you test it?

n/a

Notes for the reviewer

Should we test this class?

Checklist

@coveralls
Copy link
Collaborator

coveralls commented Jun 26, 2023

Pull Request Test Coverage Report for Build 5387621975

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1146 unchanged lines in 20 files lost coverage.
  • Overall coverage increased (+0.8%) to 43.77%

Files with Coverage Reduction New Missed Lines %
nodes/file_classifier/file_type.py 1 98.28%
nodes/file_converter/markdown.py 1 98.25%
nodes/file_converter/init.py 2 88.24%
nodes/prompt/prompt_model.py 7 85.19%
nodes/prompt/invocation_layer/sagemaker_hf_text_gen.py 11 71.43%
document_stores/es_converter.py 16 21.57%
nodes/file_converter/image.py 21 25.4%
utils/import_utils.py 25 33.33%
utils/preprocessing.py 25 8.25%
document_stores/elasticsearch.py 40 45.07%
Totals Coverage Status
Change from base Build 5376241675: 0.8%
Covered Lines: 9980
Relevant Lines: 22801

💛 - Coveralls

Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation looks very nice and clean to me.

  • Can you think of an easy way of testing it?
  • I think it should be documented somewhere.

@ZanSara
Copy link
Contributor Author

ZanSara commented Jun 27, 2023

Tests added! I decided to test only if the disable property is set, because I found really hard to figure out if the bar is being printed. On the other hand, I don't want to test tqdm itself, so this looks like a decent compromise. I also updated the PR description to clarify how to use this new feature.

Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ZanSara ZanSara merged commit 462f3a5 into main Jun 27, 2023
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable all progress bars
3 participants