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

Issue #2758 - multiprocessing.spawn submodule and multiprocessing.pool stubs #2823

Merged
merged 4 commits into from Mar 5, 2019

Conversation

Projects
None yet
2 participants
@smcl
Copy link
Contributor

commented Mar 4, 2019

While this fixes #2823, it's probably possible to do a bit more particularly in the pool stub. What I've tackled here is:

  • in the implementation of pool we have a class AsyncResult which was aliased to ApplyResult ... but our stub only had reference to the AsyncResult. I've added this and a little comment similar to what was already present in CPython's pool.py
  • no mention of the RUN, CLOSE and TERMINATE in pool (mentioned earlier in the issue)
  • new stub for spawn submodule, including the freeze_support function (mentioned earlier in the issue) and a number of others which appear to be inside this
  • the multiprocessing module's freeze_support and
    set_executable functions are now aliases of the versions inside the spawn submodule

So with a test like the following:

from multiprocessing import Manager
from multiprocessing.spawn import set_executable, get_executable, is_forking, freeze_support, get_command_line, spawn_main, _main, get_preparation_data, prepare, import_main_path, old_main_modules
from multiprocessing.pool import AsyncResult, ApplyResult, RUN, CLOSE, TERMINATE

We previously got:

2578.py:2: error: No library stub file for standard library module 'multiprocessing.spawn'
2578.py:2: note: (Stub files are from https://github.com/python/typeshed)
2578.py:3: error: Module 'multiprocessing.pool' has no attribute 'ApplyResult'
2578.py:3: error: Module 'multiprocessing.pool' has no attribute 'RUN'
2578.py:3: error: Module 'multiprocessing.pool' has no attribute 'CLOSE'
2578.py:3: error: Module 'multiprocessing.pool' has no attribute 'TERMINATE'

Running this with mypy using the typeshed which has my changes eliminates these.

smcl added some commits Mar 4, 2019

@srittau
Copy link
Collaborator

left a comment

Thank you for the pull request. Some notes below.

def get_all_start_methods() -> List[str]: ...
def get_context(method: Optional[str] = ...) -> BaseContext: ...
def get_start_method(allow_none: Optional[bool]) -> Optional[str]: ...
def set_start_method(method: str, force: Optional[bool] = ...) -> None: ...
freeze_support = spawn.freeze_support
set_executable = spawn.set_executable

This comment has been minimized.

Copy link
@srittau

srittau Mar 5, 2019

Collaborator

A better way to reexport something from another type stub is this syntax (in the import section):

from .foo import bar as bar
# undocumented
_python_exe: str

def set_executable(executable: str) -> None: ...

This comment has been minimized.

Copy link
@srittau

srittau Mar 5, 2019

Collaborator

executable -> exe (relevant if someone uses keyword arguments).

This comment has been minimized.

Copy link
@smcl

smcl Mar 5, 2019

Author Contributor

Cool, I think I took the name from the version previously defined at the top-level (multiprocessing/init.py) but in any case you're right, and the argument names should match 👍

WINEXE: bool
WINSERVICE: bool
# undocumented
_python_exe: str

This comment has been minimized.

Copy link
@srittau

srittau Mar 5, 2019

Collaborator

I don't think it's a good idea to include undocumented, private members like this, especially since accessors are readily available.

This comment has been minimized.

Copy link
@smcl

smcl Mar 5, 2019

Author Contributor

Makes sense


def set_executable(executable: str) -> None: ...
def get_executable() -> str: ...
def is_forking(argv: List[str]) -> bool: ...

This comment has been minimized.

Copy link
@srittau

srittau Mar 5, 2019

Collaborator

Looking at the implementation, requiring a Sequence instead of a List should be enough.

def is_forking(argv: List[str]) -> bool: ...
def freeze_support() -> None: ...
def get_command_line(**kwds: Any) -> List[str]: ...
def spawn_main(pipe_handle: int, parent_pid: Optional[int] = ..., tracker_fd: Optional[float] = ...) -> None: ...

This comment has been minimized.

Copy link
@srittau

srittau Mar 5, 2019

Collaborator

Is tracker_fd really a float? Just going by the argument name, I'd expect an int.

This comment has been minimized.

Copy link
@smcl

smcl Mar 5, 2019

Author Contributor

Yep this is obviously a file descriptor, no idea what happened here

def spawn_main(pipe_handle: int, parent_pid: Optional[int] = ..., tracker_fd: Optional[float] = ...) -> None: ...
# undocumented
def _main(fd: int) -> Any: ...
def get_preparation_data(name: str) -> Dict[str, str]: ...

This comment has been minimized.

Copy link
@srittau

srittau Mar 5, 2019

Collaborator

Should return Dict[str, Any].

def _main(fd: int) -> Any: ...
def get_preparation_data(name: str) -> Dict[str, str]: ...
old_main_modules: List[ModuleType] = ...
def prepare(data: Dict[str, str]) -> None: ...

This comment has been minimized.

Copy link
@srittau

srittau Mar 5, 2019

Collaborator

data should be a Mapping[str, Any].

smcl added some commits Mar 5, 2019

@srittau

srittau approved these changes Mar 5, 2019

@srittau srittau merged commit 17cd91e into python:master Mar 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.