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

Added the no-progress flag for project.py status to hide the progress… #685

Merged
merged 36 commits into from
Sep 5, 2023

Conversation

iblanco11981870
Copy link
Contributor

@iblanco11981870 iblanco11981870 commented Oct 24, 2022

Description

Added the no-progress flag for FlowProject.print_status. This hides the progress bar output generated when FlowProject.print_status, and adds the options to hide the progress bar when desired.

Motivation and Context

The progress bar displays a significant amount of the output for small workspaces and does not give any benefit. Additionally, using FlowProject.print_status in Jupyter notebooks requires special configurations or else it does not work. This addresses issue #602

Checklist:

@codecov
Copy link

codecov bot commented Oct 24, 2022

Codecov Report

Merging #685 (2918c18) into main (5093591) will increase coverage by 0.17%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #685      +/-   ##
==========================================
+ Coverage   69.10%   69.28%   +0.17%     
==========================================
  Files          43       43              
  Lines        4279     4294      +15     
  Branches     1044     1047       +3     
==========================================
+ Hits         2957     2975      +18     
+ Misses       1109     1108       -1     
+ Partials      213      211       -2     
Files Changed Coverage Δ
flow/project.py 83.12% <100.00%> (+0.01%) ⬆️
flow/util/misc.py 78.52% <100.00%> (+4.03%) ⬆️

@iblanco11981870 iblanco11981870 marked this pull request as ready for review November 8, 2022 19:00
@iblanco11981870 iblanco11981870 requested review from a team as code owners November 8, 2022 19:00
@iblanco11981870 iblanco11981870 requested review from bdice and Tobias-Dwyer and removed request for a team November 8, 2022 19:00
flow/util/misc.py Outdated Show resolved Hide resolved
flow/util/misc.py Outdated Show resolved Hide resolved
flow/util/misc.py Outdated Show resolved Hide resolved
Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @iblanco11981870! I had to rebase this PR because it contained some merged commits that were included by accident and made the PR diff appear incorrectly. Please run these commands to fix your local branch:

$ git fetch --all
$ git checkout feature/print_status
$ git reset --hard origin/feature/print_status

I have some design input as well. See comments. Thank you!

flow/project.py Outdated Show resolved Hide resolved
flow/project.py Show resolved Hide resolved
flow/project.py Outdated Show resolved Hide resolved
flow/project.py Outdated Show resolved Hide resolved
flow/project.py Outdated Show resolved Hide resolved
tests/test_status.py Outdated Show resolved Hide resolved
@joaander
Copy link
Member

joaander commented Feb 2, 2023

What more needs to be done to complete this pull request? I would like to use it to clean up the output in the HOOMD-blue tutorials.

@klywang
Copy link
Contributor

klywang commented Feb 2, 2023

@joaander I think we're just waiting on a reviewer. I addressed all the suggested changes.

@joaander
Copy link
Member

joaander commented Feb 2, 2023

OK, the reviewers may not know unless you click the "re-request review" button or otherwise communicate that to them.

@bdice
Copy link
Member

bdice commented May 26, 2023

@iblanco11981870 Can you please resolve conflicts on this branch before I review? Thanks!

changelog.txt Outdated Show resolved Hide resolved
flow/project.py Outdated
progress : bool
Show a progress bar during execution.
hide_progress : bool
Hides progress bar when printing status output (Default value = True).
Copy link
Member

Choose a reason for hiding this comment

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

There is no default value set here.

Copy link
Member

Choose a reason for hiding this comment

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

Has been reverted.

flow/project.py Outdated
Comment on lines 5053 to 5054
"--hide_progress",
action="store_false",
Copy link
Member

Choose a reason for hiding this comment

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

Trying to make sure I understand here -- why is this store_false and the other line above is store_true? Does the progress behavior match the behavior prior to this PR?

Copy link
Member

Choose a reason for hiding this comment

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

This did change behavior and has been removed.

tests/test_status.py Show resolved Hide resolved
@bdice
Copy link
Member

bdice commented Jul 12, 2023

@iblanco11981870 Can we push this through or close it?

Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

@b-butler Thanks for requesting my review on this. I took one brief pass but can't dedicate more time to this PR. Please merge when you think it is sufficiently ready.

flow/util/misc.py Outdated Show resolved Hide resolved
@b-butler b-butler merged commit c65cc0f into main Sep 5, 2023
12 checks passed
@b-butler b-butler deleted the feature/print_status branch September 5, 2023 23:16
@joaander
Copy link
Member

joaander commented Sep 6, 2023

Thanks!

@b-butler b-butler modified the milestones: v1.0, v0.26.0 Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants