pallets / click

Python composable command line interface toolkit
https://click.palletsprojects.com
BSD 3-Clause "New" or "Revised" License
15.78k stars 1.4k forks source link

Progressbar do not advance with `.next()` call #1556

Closed georgexsh closed 4 years ago

georgexsh commented 4 years ago

Expected Behavior

with the following code:

import time
import click

lst = list(range(10))
with click.progressbar(lst) as bar:
    for i in lst:
        next(bar)
        time.sleep(0.2)

bar just don't advance, related to https://github.com/pallets/click/issues/1125.

Actual Behavior

works fine with click-6.7.

Environment

obfusk commented 4 years ago

I don't think this is a bug in click. You're calling next() on something that may not be an iterator.

I'm also not sure why you're iterating over lst instead of bar. The following works fine:

import time
import click

lst = list(range(10))
with click.progressbar(lst) as bar:
    for i in bar:
        time.sleep(0.2)

If, for some reason, you really want to iterate over lst instead of bar, you can call next() on an iterator over bar (obtained using iter()):

import time
import click

lst = list(range(10))
with click.progressbar(lst) as bar:
    it = iter(bar)
    for i in lst:
        next(it)
        time.sleep(0.2)
davidism commented 4 years ago

@obfusk thanks for the good explantion :-)

georgexsh commented 4 years ago

@obfusk I see, you mean in click 7, click.progressbar is not an iterator anymore? but in the meanwhile, it has the .next/.__next__ method, and bar.next() is broken.

georgexsh commented 4 years ago

@obfusk besides, with your code, the progress bar will not reach 100%:

$ python test_bar.py
  [################################----]   90%  00:00:00
obfusk commented 4 years ago

@obfusk besides, with your code, the progress bar will not reach 100%:

You got me there. It's missing the final step. This does work:

import time
import click

lst = list(range(10))
with click.progressbar(lst) as bar:
    it = iter(bar)
    for i in lst:
        next(it)
        time.sleep(0.2)
    next(it, None)

But the first example I gave of just iterating over bar in the for loop -- and not calling next() manually -- works just fine and is the intended use case.

obfusk commented 4 years ago

@obfusk I see, you mean in click 7, click.progressbar is not an iterator anymore? but in the meanwhile, it has the .next/.__next__ method, and bar.next() is broken.

Some iterables are their own iterators. Many are not. Which is why we have iter(): to get an iterator for an iterable. This is also what a for loop uses implicitly. Using click.progressbar as an iterator directly -- instead of calling iter() to get one -- may have worked in the past but was never guaranteed to work.

obfusk commented 4 years ago

So you're right: in click 7, click.progressbar is no longer an iterator. That it happened to be its own iterator before is an implementation detail. It was always only guaranteed to be an iterable.

obfusk commented 4 years ago

And if you really want to iterate over something other than bar, you're probably looking for .update() (and length=) instead of next():

import time
import click

lst = list(range(10))
with click.progressbar(length = len(lst)) as bar:
    for i in lst:
        time.sleep(0.2)
        bar.update(1)
georgexsh commented 4 years ago

@obfusk thanks for your help, I have already switched to .update(), but It's really confusing to me.

So you're right: in click 7, click.progressbar is no longer an iterator. That it happened to be its own iterator before is an implementation detail. It was always only guaranteed to be an iterable.

I guess this worth be documented? or click.progressbar could just remove __next__ method, if it not meant to be an iterator, let next(bar) call break.

davidism commented 4 years ago

We literally just had an issue from the previous release that asked us to add it back. I'm not interested in messing with this interface anymore, it just keeps getting more complicated for questionable benefit. If you need a very customizable progress bar, use tqdm.

obfusk commented 4 years ago

I think there might be some confusion regarding the progressbar interface. Which would explain why people (try to) call next() instead of just iterating over the bar.

The following two programs are functionally equivalent. But the second one doesn't make much sense. If you want to process the items from the iterable passed to click.progressbar, you should not iterate over the original iterable! The progressbar wraps that original iterable in a new iterable that iterates over the same values and also advances the progressbar.

import click, time
processed_items = []
def process(item):
    processed_items.append(item)
items = "foo bar baz qux".split()
with click.progressbar(items) as bar:
    for item in bar:
        process(item)
        time.sleep(1)
print("processed items:", *processed_items)
import click, time
processed_items = []
def process(item):
    processed_items.append(item)
items = "foo bar baz qux".split()
with click.progressbar(items) as bar:
    it = iter(bar)
    for item in items:
        same_item = next(it)
        assert item == same_item
        process(item)
        time.sleep(1)
    next(it, None)
print("processed items:", *processed_items)

@georgexsh if this explanation still doesn't help with your confusion, I don't think this is the right place to discuss this further. I'd suggest you try asking on stack overflow (or send me an email if you want).

@davidism if this is indeed the source of confusion, maybe the documentation could be improved?

davidism commented 4 years ago

Happy to consider improvements to the docs. It sounds like you've built up some good material at this point!

georgexsh commented 4 years ago

@obfusk thanks for the explanation, as the prgressbar do yield item before update(1), one should exhaust the iterable with next(it, None), to make the bar reach 100%. I would stick with update, but it would be very great if those notes are included in the document.