Allow passing a --no-progress-bar to the install script to surpress progress bar

[AvnerCohen]

Might probably be in complete, first time on this code base, happy to fix whatever needed.

This should fix things like #2369 and #2756

---

This change is 

[pfmoore]

Thanks for doing this! It mostly looks good to me.

Could you fix the PEP8 issues, and add some tests? Also, I'm not 100% comfortable with the double negatives in the no_progress_bar=False arguments - could you restructure to use progress_bar=True? I'd also like some examples (either in docs or in tests) of how this would look in config files - we've got other cases where we have weird interactions. Ideally, I'd like usage to be:

• Command line: pip install --no-progress-bar foo
• Config file: progress-bar=no
• Environment variable: PIP_PROGRESS_BAR=FALSE

Whether that's straightforward with our current code to handle config, I'm not sure. If not, then I'm OK with a workable compromise, but I'd like the behaviour to be clearly spelled out.

[dstufft]

I wonder if it makes sense to make this a enum instead of a boolean? E.g. instead of --no-progress-bar, have --progress-bar [on|off|ascii|pretty]. Where it defaults to on. That completely side steps the double negative issue and it makes this mechanism useful for people who have problems with the "pretty" bar where we aren't correctly falling back to a pure ASCII bar.

[pradyunsg]

Whether that's straightforward with our current code to handle config, I'm not sure.

AFAICT, It is. I've been fiddling with that code. Just adding an option on the command makes it available to the entire configuration setup.

[AvnerCohen]

@pfmoore Thank you !
I've tried to understand how to best remove the double negation, I think in the command itself, it makes it very clear to have the "no(-progress-bar)" so I've done the negation once in install.py:

                    progress_bar=(not options.no_progress_bar),

Not perfect, but I felt this is the best mid way I could find.

I was a bit unsure about where and how to plug configuration file options, if you think this is important, I can spend more time on that.

@dstufft
Regarding the option to use enums here, I don't know enough to say if this is right or wrong in the "pip" spirit, but looking at other tools, there is usually a flag you want to have to silent progress bar for non tty or CI type systems, I think overloading a "progress selector" flag for that would be less intuitive.

[pfmoore]

In principle I agree with @dstufft - letting the option choose between "graphical", ASCII or no progress bar is likely to be useful. My only concern is that it's over-generalising. It does nicely avoid the whole no-progress=no thing, though :-)

[pfmoore]

Note that in #4196 (an alternative PR for this) the option affected pip wheel and pip download as well. Whatever the option is, it should act consistently wherever a progress bar is shown.

[dmytrokyrychuk]

@AvnerCohen progress bar might also be shown during download and wheel commands execution. You can find the places where the flag should also be specified in this commit: de0e9ec.

---

BTW, most of the other flags names begin with a verb: use_user_site, ignore_requires_python, etc. I think it might be reasonable to rename all the progress_bar arguments to allow_progress_bar.

[AvnerCohen]

Thanks @orgkhnargh funny coincidence :) I'll add the other commands as you suggested.
Working now to try and apply @dstufft for multi value setup.

Will also add testing.

[AvnerCohen]

@dstufft, @orgkhnargh, @pfmoore
I did not rebase to make it easier to review changes, also did not yet added tests and cleanup some code duplications.

Unlike the original rather minor change, this is much more elaborate change, I'd appreciate preliminary thoughts on this directions before moving on the finalize this spike.

Two things to note:

1. "Spinner" is also a progress we need to eliminate in the silent mode, so add logic around that as well.
2. Given the new direction, I thought it makes sense to refer to the "silent" mode with a NullPattern object, so a SilentBar was created to work with the Bar api.

10x.

[dstufft]

I think the idea of implementing the silent mode with a null object makes sense, but I think that rather than giving people the option to select specific named bars (or even force everything to a spinner), using a dictionary like:

BAR_TYPES = {
    "off": (SilentBar, SilentBar),
    "ascii": (Bar, Spinner),
    "pretty": (IncrementalBar, Spinner),  # If there is a pretty spinner we can add, use it instead
}

Then we can use BAR_TYPES.keys() for choices, and for progress bars we can use BAR_TYPES[type][0] and spinners we can use BAR_TYPES[type][1]. The only downside to this is that it's a little harder to implement the pretty -> ascii fallback that currently happens if we can't encode our characters, but that should be able to be implemented by adding another type that's just "on" which does that for us.

[dmytrokyrychuk]

@dstufft I'm not sure I follow. Are you suggesting to implement two options: one to choose among {"on", "off", "ascii", "pretty"} and one to choose among {"bar", "spinner"}?

UPD: nevermind, I found out what are spinners used for. Sorry.

[dstufft]

@orgkhnargh No sorry, end users only choose between on/off/ascii/pretty. The system choses between the bar or spinner depending on if there is a known, finite end to the progress (e.g. a download of size X) or if it's unknown when it will complete (e.g. setup.py execution).

[AvnerCohen]

@dstufft Implemented this.
I liked your idea !

[AvnerCohen]

@dstufft @pfmoore
Any thoughts on this?
Shall I squash and maybe add something to the docs?

[xavfernandez]

Also add (default: %(default)s) in the help string.

[xavfernandez]

This looks very good 👍
Please also add the option to pip/commands/download.py (+ the default value in the help string).
A squash, a changelog and this can be merged :)

PS: you have a typo in your commit message (surpress vs suppress).

[AvnerCohen]

Thanks for the review @xavfernandez , hopefully this is covered now, do let me know if anything is still missing.

[xavfernandez]

You can directly pass %default in the help string, cf https://github.com/pypa/pip/blob/master/pip/cmdoptions.py#L222

[AvnerCohen]

Oh.. sorry about that, kind of missed that one due to the other interpolation.
Fixed now.

[xavfernandez]

The option is --progress-bar and is also added to pip wheel :)
Sorry I did not spot this on the last review.

[AvnerCohen]

Good spotting!
Apologies for the mix-up and thanks for keeping this clean and tidy !

[xavfernandez]

Thanks a lot :)

[jaap3]

First of all thanks for the patch and getting it merged! We'll start using this flag as soon as possible in our CI scripts :)

I just have one question about the 'emoji' progress bar. It seems like a strange addition, why was it added?

Also, a minor nitpick, the instruction:

"Add --progress-bar <progress_bar> to ... to suppress progress bar in the console"

Should probably just say "Add --progress-bar off" as using any other value will not actually suppress the progress bar.

[AvnerCohen]

@jaap3 Glad this is of help. Our CI will benefit just as well, hopefully this will improve electricity consumption across the world with widely distributed reduced logging :)

1. The emoji is more of a stub/POC to alternative progress bars, not sure how and if this can be beneficial, I thought it's worth it given there is no harm and it's totally about to the user to use it or not.
2. That's a good point regarding the change log, the help line is more accurate:

  --progress-bar <progress_bar>
      Specify type of progress to be displayed [on|ascii|off|pretty|emoji] (default: on)

It's a result of the fact the plan was to originally have a boolean value, we ended up doing a multi-value choice.

Not sure if it's worth altering the changelog, I can defiantly patch that if it's worth while.
@xavfernandez thoughts on that?

[xavfernandez]

Sure, if you can make the changelog clearer, go ahead :)

[memory]

Is there any sort of timeline for this making it into a release? The 9.1 milestone seems very far away at the moment, and there are many, many automated builds around the world that would become far more readable if this flag were available. :)

[AvnerCohen]

Ping & sorry to nag, any chance of this going into a release anytime soon?

[AvnerCohen]

For anyone interested, a potential workaround to suppress progress bar until this one is published:

pip install --no-cache-dir -U -r requirements.txt | cat

[santiagax99]

@AvnerCohen with your workaround any install fails will return 0 because of "pipe cat". So there is potential risk using this method in complex bash scripts even with set -e option

[dmytrokyrychuk]

@santiagax99 you can use set -o pipefail as a workaround together with @AvnerCohen's workaround :)

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.