The timeout argument of ProcessPool().submit() is inconsistent with ThreadPool().submit()

[victoryhb]

The timeout argument of ProcessPool().submit() is inconsistent with ThreadPool().submit()

[noxdafox]

Goal with pebble is not to be concurrent.futures compatible. Only in recent versions I adopted a somewhat similar API to make it more friendly and also to re-use the Future class.

I find the Executor API pretty simplistic and I don't think that adding a custom namespace _pebble_specific_timeout is the correct solution.

I will reinstate the schedule API as the recommended function for submitting jobs to the Pools and leave the submit for mere compatibility with asyncio.loop.run_in_executor.

Main point here is that there are 2 sets of parameters/arguments:

1. The ones which are Pool specific (what to run and how)
2. The ones that are function specific (the function args and kwargs)

The two should be logically separated and not intertwined.

[noxdafox]

Hello,

the pool.submit has been designed to be compatible with concurrent.future.Executor.submit for the sake of supporting asyncio.loop.run_in_executor API.

The concurrent.future.Executor.submit API passes arguments and keyword arguments straight to the submitted function making positional arguments the only way to provide submission specific parameters.

This is evident in the function signature Executor.submit(fn, /, *args, **kwargs) note the / marking positional only parameters.

It would not be possible, in fact, to provide a signature such as ProcessPool.submit(fn, *args, timeout=None, **kwargs) as this would prevent submitting functions with a timeout keyword arguments such as requests.get. Moreover, the above API would not allow to submit jobs within asyncio.loop.run_in_executor as it would not be possible to submit them altogether with a timeout.

If you want to use ThreadPool and ProcessPool interchangeably and you don't care about the timeout functionality then you can simply use a class wrapper for it.

class Pool:
    def __init__(pool: [pebble.ProcessPool, pebble.ThreadPool]):
        self._pool = pool

    def submit(fn, *args, **kwargs):
         if isinstance(self._pool, pebble.ProcessPool):
            self._pool.submit(fn, None, *args, **kwargs)

pool = Pool(pebble.ProcessPool())
futures = [pool.submit(func, i) for i in range(10)]

Keep in mind that ThreadPool and ProcessPool do not provide the same behaviour. Futures returned by ProcessPool can be cancelled and can timeout. In such cases, the underlying process will be interrupted avoiding to consume extra resources. The same is not true for ThreadPool due to the fact that running threads cannot be interrupted. Hence, you need to keep this aspect in mind when exchanging one pool with the other.

[victoryhb]

Thanks a lot for the enlightening remarks on the rationale behind such a design decision.
I see that the positional timeout argument was added mainly to be compatible with libs like asyncio.
Coming from concurrent.futures, however, I was surprised by the subtle incompatibity in the API. I personallly would have preferred full compatibility by adding a special keyword argument like pebble_timeout, which also avoids the somewhat redundant use of None in many cases.