Implement CustomProgress that does not output empty divs when disabled

tomicapretto commented 2 months ago

Description

This PR adds a new class CustomProgress that inherits from rich.progress.Progress. I overloaded the necessary methods to make it not output anything when we disable the progress bar. I feel it's a bit hacky so I would like to know what others think about this modification.

This problem is noticeable especially in VS Code. It's not such a big issue in Jupyter Notebook. Some screenshots:

VS Code progressbar=True (before and after we get the same result, I cannot get rid of those empty spaces)

capture1

VS Code progressbar=False before the changes

capture2

VS Code progressbar=False after the changes

Jupyter Notebook after the changes

capture3

Update

In the terminal, after the change

Related Issue

Closes #7264

📚 Documentation preview 📚: https://pymc--7290.org.readthedocs.build/en/7290/

tomicapretto commented 2 months ago

Some tests are failing, it's because there's something I missed in the child class I created.

tomicapretto commented 2 months ago

@ricardoV94 this is ready to be reviewed

ricardoV94 commented 2 months ago

@tomicapretto you seem to have picked commits from main, can you rebase? Hard to review otherwise

tomicapretto commented 2 months ago

@tomicapretto you seem to have picked commits from main, can you rebase? Hard to review otherwise

I think I did not the cleanest thing but I think it does not show anything from main now

tomicapretto commented 2 months ago

Converted to a draft because I found other points of improvement.

I think the refresh argument is not needed and it causes the sampler to be slower
There should be some kind of final update when computing predictions, see the bar is not full:

fonnesbeck commented 2 months ago

The refresh argument was added because (1) without it progress bars did not always end at 100% after sampling was complete (as you noted above) and (2) the divergences text did not update without it.What about adding a single refresh update at the end of sampling? Perhaps that would ensure the last update is complete.

You can use the example in #7278 to test for (2).

tomicapretto commented 2 months ago

@fonnesbeck I'll test some ideas / combinations and update on my findings

drbenvincent commented 2 months ago

FYI: This has also affected doctests in CausalPy, see https://github.com/pymc-labs/CausalPy/issues/323 Would be very pleased to see a fix :)

tomicapretto commented 1 month ago

Before the last commit, it wouldn't update to 100% using one chain (because it doesn't use multiprocess sampling)

completed_not_full

Now it works well

I can confirm divergences are updated and it goes up to 100%

The progressbar for posterior predictive sampling was also broken: posterior_predictive_not

Now it works fine: posterior_predictive_ok

tomicapretto commented 1 month ago

I'm now trying to see if I can get those two extra divs not added. It does not take any relevant space when using jupyter notebook, but it appears on VS Code and it's really bothering.

tomicapretto commented 1 month ago

Ok I have some findings. It has to do with how rich handles the output in jupyter notebooks. Console has a force_jupyter parameter which is None by default (and with notebooks it's set to True internally). See what happens when we change it.

In VS Code, it removes the extra space that gets added, but notice it changes the color and the description: force_jupyter_vs

In VS Code, it doesn't do any harm, but it stillchanges the color and the description: force_jupyter_jpt

I'm not sure if this is something we can take care of. It's really bothering for the ones who use VS Code, but I don't see any simple solution. I'll do a bit more research but I'm not going to invest a lot of time on it.

EDIT

It's not that it does not produce the same output on both platforms. It does produce the same output and it's this:

<pre style="white-space:pre;overflow-x:auto;line-height:normal;font-family:Menlo,'DejaVu Sans Mono',consolas,'Courier New',monospace"></pre>

But I guess Jupyter Notebooks must be using some CSS that causes the height of the output to be very small, while VS Code does not.

ricardoV94 commented 1 month ago

Not to be a bummer, but should we revert to the old progessbar? @fonnesbeck @zaxtax ?

tomicapretto commented 1 month ago

Not to be a bummer, but should we revert to the old progessbar? @fonnesbeck @zaxtax ?

I'm not sure what was the reason to move away from the old one. But definitely the goodness from rich are not for free.