Skip to content

Conversation

donBarbos
Copy link
Contributor

@donBarbos donBarbos commented May 20, 2025

@donBarbos
Copy link
Contributor Author

cc @picnixz

@picnixz picnixz added the needs backport to 3.14 bugs and security fixes label May 20, 2025
:func:`pipe <os.pipe>`.

.. class:: InterpreterPoolExecutor(max_workers=None, thread_name_prefix='', initializer=None, initargs=(), shared=None)
.. class:: InterpreterPoolExecutor(max_workers=None, thread_name_prefix='', \
Copy link
Member

Choose a reason for hiding this comment

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

This is unrelated, but it's probably an improvement as it's now more readable. So I'm ok with it.

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I don't know if the parameters are meant to be public but the signature indeed needs to be correct for possible subclassing.

However, prepare_context is not documented so we can't even know how to use them. So we need to also document this at the same time.

@picnixz picnixz requested a review from ericsnowcurrently May 20, 2025 12:22
@donBarbos
Copy link
Contributor Author

I don't know if the parameters are meant to be public but the signature indeed needs to be correct for possible subclassing.

ctxkwargs argument described in ThreadPoolExecutor.__init__ docstring.

And maybe I'm missing something but this is what the class method prepare_context looks like (I don't know how **ctxkwargs should be passed here if this argument is omitted in prepare_context):

@classmethod
def prepare_context(cls, initializer, initargs):
return WorkerContext.prepare(initializer, initargs)
def __init__(self, max_workers=None, thread_name_prefix='',
initializer=None, initargs=(), **ctxkwargs):
"""Initializes a new ThreadPoolExecutor instance.
Args:
max_workers: The maximum number of threads that can be used to
execute the given calls.
thread_name_prefix: An optional name prefix to give our threads.
initializer: A callable used to initialize worker threads.
initargs: A tuple of arguments to pass to the initializer.
ctxkwargs: Additional arguments to cls.prepare_context().
"""
if max_workers is None:
# ThreadPoolExecutor is often used to:
# * CPU bound task which releases GIL
# * I/O bound task (which releases GIL, of course)
#
# We use process_cpu_count + 4 for both types of tasks.
# But we limit it to 32 to avoid consuming surprisingly large resource
# on many core machine.
max_workers = min(32, (os.process_cpu_count() or 1) + 4)
if max_workers <= 0:
raise ValueError("max_workers must be greater than 0")
(self._create_worker_context,
self._resolve_work_item_task,
) = type(self).prepare_context(initializer, initargs, **ctxkwargs)

@picnixz
Copy link
Member

picnixz commented May 20, 2025

And maybe I'm missing something but this is what the class method prepare_context looks like (I don't know how **ctxkwargs should be passed here if this argument is omitted in prepare_context):

in this case, we should wait for Eric's feedback. I'm away until Saturday so I won't be able to reply in the meantime.

class method.

.. classmethod:: prepare_context(initializer, initargs)
Setting up the necessary context for creating worker instances in a pool-based executor (e.g., in a concurrent environment like threads or interpreters).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Setting up the necessary context for creating worker instances in a pool-based executor (e.g., in a concurrent environment like threads or interpreters).
Setting up the necessary context for creating worker instances in a pool-based executor (for example, in a concurrent environment like threads or interpreters).

Use simple language and this line needs wrapping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review docs Documentation in the Doc dir needs backport to 3.14 bugs and security fixes skip news

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

3 participants