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.

Sign up for GitHub

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

Jump to bottom

Support multiple spinners running in parellel #155

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Support multiple spinners running in parellel #155

wants to merge 7 commits into from
+94 −14

Conversation

frostming
Copy link

@frostming frostming commented Oct 28, 2020 •
edited
Loading

Description of the new feature, or changes

Add multi-threading support so that spinners can be updated individually without affecting other instances.

Example script:

import random
import time
from concurrent.futures import ThreadPoolExecutor

from halo import Halo


def long_running_task(i):
    with Halo(f"Running {i}", spinner="dots") as sp:
        time.sleep(3 + random.random())
    sp.succeed(f"Success {i}")


with ThreadPoolExecutor(max_workers=4) as pool:
    # Number of tasks is greater than max workers
    pool.map(long_running_task, range(8))

And here is the output

test

Checklist

  • Your branch is up-to-date with the base branch
  • You've included at least one test if this is a new feature
  • All tests are passing

Related Issues and Discussions

Close #135
Close #17

halo_notebook is not updated. Tests are not supplied yet, any suggestions are welcome.

People to notify

@frostming
Copy link
Author

/cc @adamtheturtle @manrajgrover @CoziestYew804

@adamtheturtle
Copy link
Contributor

@frostming Thanks for mentioning me, and the gif looks great. I won't review as I don't have the expertise but I hope that this, or similar, makes it into halo!

@manrajgrover manrajgrover self-requested a review October 29, 2020 11:20
@manrajgrover
Copy link
Owner

@frostming Thanks for working on this. It looks great!

I'll review this over the weekend. But this does require testing and support for notebook environment.

@coveralls
Copy link

coveralls commented Oct 29, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 442

  • 39 of 40 (97.5%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.5%) to 84.584%
Changes Missing Coverage Covered Lines Changed/Added Lines %
halo/halo.py 39 40 97.5%
Totals Coverage Status
Change from base Build 440: 1.5%
Covered Lines: 334
Relevant Lines: 388
💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 441

  • 38 of 39 (97.44%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.1%) to 84.146%
Changes Missing Coverage Covered Lines Changed/Added Lines %
halo/halo.py 38 39 97.44%
Totals Coverage Status
Change from base Build 440: 1.1%
Covered Lines: 333
Relevant Lines: 387
💛 - Coveralls

@manrajgrover manrajgrover mentioned this pull request Oct 29, 2020
Open
@frostming
Copy link
Author

I added a test to cover the basic scenario of multiple running spinners.

But this does require testing and support for notebook environment.

Notebook should be working fine without any changes because instances are rendered in their own output widget, which I confirmed in a local notebook.

@frostming
Copy link
Author

@frostming Thanks for mentioning me, and the gif looks great. I won't review as I don't have the expertise but I hope that this, or similar, makes it into halo!

Hi, Any progress on the review?

@edgarcosta
Copy link

For some reason on the notebook very often one of the widgets gets overwritten:
halo

and in the console I see a lot of:

Popped wrong message (xxx instead of yyy) - likely the stack was not maintained in kernel.

TheFenrisLycaon
TheFenrisLycaon approved these changes May 17, 2021
View reviewed changes
Copy link

@TheFenrisLycaon TheFenrisLycaon left a comment

Choose a reason for hiding this comment

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

Seems alright to me.

@phylroy
Copy link

phylroy commented Jul 22, 2021

Any news on this being merged in? Looks like a nice addition.

@mrkmndz
Copy link

mrkmndz commented Sep 24, 2021

@manrajgrover is there any reason this PR hasn't been approved? Is this project actively maintained anymore?

@edgarcosta
Copy link

edgarcosta commented Sep 27, 2021 via email

@ostanislaw
Copy link

Very good change. I also would like several spinners for multiprocessing.

@frostming frostming mentioned this pull request Nov 18, 2021
Closed
9 tasks
@black-snow
Copy link

Still not merged? I was looking for exactly this. If anyone is reading this and has found an alternative to halo that works with multiple spinners, please ping me.

@frostming
Copy link
Author

Still not merged? I was looking for exactly this. If anyone is reading this and has found an alternative to halo that works with multiple spinners, please ping me.

@black-snow I think this project is no longer maintained, have a look at rich you may achieve the same using rich's spinner and Live Display

@nmacholl
Copy link

nmacholl commented Aug 3, 2022

@frostming thanks for the work you put into this PR; unfortunate the project isn't maintained.

@edgarcosta edgarcosta mentioned this pull request May 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheFenrisLycaon TheFenrisLycaon TheFenrisLycaon approved these changes

@manrajgrover manrajgrover Awaiting requested review from manrajgrover

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Overwriting Spinners Provide option to render more than one spinner at the same time