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

Semaphore is not implemented. #524

Closed
ghost opened this issue Sep 21, 2018 · 8 comments
Closed

Semaphore is not implemented. #524

ghost opened this issue Sep 21, 2018 · 8 comments

Comments

@ghost
Copy link

ghost commented Sep 21, 2018

Relates to #388

Operating system: Android 8.0.0 (OnePlus 3T, Termux)
Python version: 3.6.6 3.6.5
Black version: 18.6b4
Does also happen on master: yes

It has come to my attention that there is no semaphore synchronization primitive on Android. This means that black will not work on this platform whatsoever, which is disappointing, especially as I find this to be a fairly useful tool that I use regularly. I cannot get the traceback off of my phone while at work unfortunately, but I believe this issue has been put in before. It occurs on line 341 of black.py when initialising a ProcessPool. In master, this is at https://github.com/ambv/black/blob/master/black.py#L343

I have tested this in the Python console with both the multiprocessing pool and the concurrent.futures process pool. Both complain about said missing synchronization primitives.

Python issue #3770 discusses this issue, which appears to not just occur on Android, but also on the BSDs. This would mean most likely (haven't tested as I don't use BSD) that both Android and BSD cannot support Black.

Whilst this is not ideal, the following could be a work around for ensuring that this package would actually still work on these platforms. Again, I haven't tested, but both TPE and PPE in concurrent.futures share the same interface.

import concurrent.futures
...
def generate_pool(*args, **kwargs) -> concurrent.futures.Executor:
    try:
        return concurrent.futures.ProcessPoolExecutor(*args, **kwargs)
    except ImportError:
        return concurrent.futures.ThreadPoolExecutor(*args, **kwargs)
...
# 344
executor = generate_pool(max_workers=os.cpu_count())

Alternatively, could we just run the tasks in series if this issue is detected, rather than using a pool to perform multiple things at once? As much as it is an inconvenience for things to run slowly or inefficiently, I would much prefer that and be able to use this tool at least somewhat on this platform.

Would this be worth considering?

Cheers 😄

@JelleZijlstra
Copy link
Collaborator

I think it makes sense to fall back to processing files serially when there is no working process pool implementation. I'd prefer not to use a thread pool, because it is unlikely to provide a speedup over serial execution.

@ghost
Copy link
Author

ghost commented Sep 21, 2018 via email

@ambv
Copy link
Collaborator

ambv commented Sep 25, 2018

Thanks for your report! This is a big surprise to me that there are attempts to run Black on Android :-)

Also it will likely still provide some speedup in environments that dont have the constraint of a GIL like CPython does.

There are currently no alternative runtimes implementing Python 3.6+ which don't have a GIL.

I think we could probably easily provide a SerialExecutor implementation which, while not very fast, would solve the compatibility problem. I would want to avoid TPE because Black was not designed with careful thread-safety in mind. It should be fine as we isolate the state to function arguments but we might be missing something.

By the way, do you have a way to test if your TPE suggestion makes Black run?

@ghost
Copy link
Author

ghost commented Sep 25, 2018 via email

@ghost
Copy link
Author

ghost commented Sep 26, 2018

See #533
screenshot_20180926-074714__01

Fixes the issue for me :)

@aureliojargas
Copy link

More than a year after the original report, I have tried in a newer environment:

  • Android 10 (app Termux)
  • Device: Google Pixel 2 XL
  • Python 3.7.4
  • Black 19.3b

I confirm that the issue still persists.

I confirm that @ghost patch still works! 🎉

@waynew
Copy link

waynew commented Nov 22, 2019

Same here - but just a regular Google Pixel.

Some of us are crazy about writing code in strange places where our laptops just don't go 😄

@MatthewScholefield
Copy link
Contributor

Btw @JelleZijlstra we can close this ticket now; idk when but the fix for this has been merged into master a while ago (and also of course recently in #2631 a fix for the fix was merged).

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

No branches or pull requests

5 participants