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

Added initial rework of the concurrent.futures module #5646

Merged
merged 33 commits into from
Sep 3, 2021
Merged

Added initial rework of the concurrent.futures module #5646

merged 33 commits into from
Sep 3, 2021

Conversation

HunterAP23
Copy link
Contributor

@HunterAP23 HunterAP23 commented Jun 16, 2021

Added most of the missing sections present in the concurrent.futures module for the process.pyi and thread.pyi files, related to #4641.

I'm sure there are plenty of edits needed for formatting reasons (like using generic types or TypeVars) so please let me know what should be added/fixed!

@github-actions

This comment has been minimized.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

(not a full review) just a couple scattered notes:

  • It would be good to preserve facets of the existing stubs, e.g. _WorkItem being Generic
  • Similarly, prefer inheriting methods from BaseExecutor over redefining them with the same type. That way shutdown will have the right signature for earlier versions of Python than 3.9 as well
  • ThreadPoolExecutor uses queue, not multiprocessing.queues

@HunterAP23
Copy link
Contributor Author

(not a full review) just a couple scattered notes:

  • It would be good to preserve facets of the existing stubs, e.g. _WorkItem being Generic
  • Similarly, prefer inheriting methods from BaseExecutor over redefining them with the same type. That way shutdown will have the right signature for earlier versions of Python than 3.9 as well
  • ThreadPoolExecutor uses queue, not multiprocessing.queues

Thank you for notes!
Just some questions as I continue working on this:

I'm not very well-versed in using Generics and TypeVars so I'm not sure how/what to do with them - I tried fixing them up in a follow-up commit, but not sure what the naming conventions and standards are for them (the docs are kind of lacking and there aren't any in-depth tutorials out there)

As for inheritance, how would I do that in the pyi files? Would I just not have a definition for submit inside the for ProcessPoolExecutor class within the process.pyi file for it to inherit that method from BaseExecutor?

As for the queues, I fixed them in another commit.

Thanks again for taking a look!

@github-actions

This comment has been minimized.

@Akuli
Copy link
Collaborator

Akuli commented Jun 16, 2021

I haven't looked at your code, but I'll try to explain when to use generics (and typevars). A simple example is a list: annotating something as list will work (with warnings), but annotating as list[str] is much more useful. To make this work, class list in stdlib.pyi is defined like class list(..., Generic[_T]), where _T is a TypeVar.

Basically, a TypeVar is useful for a class that contains other values of arbitrary type, but they are always of the same type. If the wrapped value can have different types, then generics don't help. For example, {"a": 123, "b": "lol"} is best annotated as dict[str, int | str] or dict[str, Any], and adding a type variable as in dict[str, _T] doesn't help.

Inheritance in .pyi files works just like you'd expect: you use the usual Python syntax, and you can import from different files.

For naming conventions, feel free to look at existing stubs and make pull requests that improve our CONTRIBUTING.md. Also, don't worry about it too much! We will point out if your naming isn't conventional enough.

@HunterAP23
Copy link
Contributor Author

I haven't looked at your code, but I'll try to explain when to use generics (and typevars). A simple example is a list: annotating something as list will work (with warnings), but annotating as list[str] is much more useful. To make this work, class list in stdlib.pyi is defined like class list(..., Generic[_T]), where _T is a TypeVar.

Basically, a TypeVar is useful for a class that contains other values of arbitrary type, but they are always of the same type. If the wrapped value can have different types, then generics don't help. For example, {"a": 123, "b": "lol"} is best annotated as dict[str, int | str] or dict[str, Any], and adding a type variable as in dict[str, _T] doesn't help.

For naming conventions, feel free to look at existing stubs and make pull requests that improve our CONTRIBUTING.md. Also, don't worry about it too much! We will point out if your naming isn't conventional enough.

Awesome, I get it a bit better now. I'll keep working on it and hopefully have it set up. I've been going over the CONTRIBUTING.md file but I'll definitely keep checking it as I write more.

Inheritance in .pyi files works just like you'd expect: you use the usual Python syntax, and you can import from different files.

I'm not sure if this ends up being a limitation of pyright, but when specifying the location of my typeshed fork for it, I get the error: Cannot access member "submit" for type "Pool" if I don't explicitly define a submit function for the ProcessPoolExecutor class in the concurrent.futures.process.pyi - it seems that it's not inheriting the submit method from yjr _base.Executor class.
Th error disappears if I keep the explicit definition of the submit method though.

@github-actions

This comment has been minimized.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

A few notes on things I have spotted, not a full review.

Comment on lines 19 to 20
_FUTURE_STATES: Sequence[str]
_STATE_TO_DESCRIPTION_MAP: Mapping[str, str]
Copy link
Collaborator

Choose a reason for hiding this comment

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

As these have concrete values (and are not supposed to be overwritten), we should use List and Dict, instead of the abstract classes.

Comment on lines 13 to 15
_WI = TypeVar["_WI"]
_RI = TypeVar["_RI"]
_CI = TypeVar["_CI"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Type vars use round parenthese: _WI = TypeVar("_WI").

Comment on lines 2 to 3
import multiprocessing.connection as mpconn
import multiprocessing.context as mpcont
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just use from abc import Whatever, unless an import conflicts with a name in this module.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Contributor Author

@HunterAP23 HunterAP23 left a comment

Choose a reason for hiding this comment

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

Re-reviewed, fixed, and re-tested successfully

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Thanks, remarks below.

stdlib/concurrent/futures/process.pyi Outdated Show resolved Hide resolved
stdlib/concurrent/futures/process.pyi Outdated Show resolved Hide resolved
stdlib/concurrent/futures/process.pyi Outdated Show resolved Hide resolved
stdlib/concurrent/futures/thread.pyi Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

2 similar comments
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@srittau
Copy link
Collaborator

srittau commented Sep 2, 2021

Maybe we should use my _ResultItem = Any suggestion to silence pyright. Alternatively, we could add concurrent.futures to the pyright.stricter config file.

@github-actions

This comment has been minimized.

@HunterAP23
Copy link
Contributor Author

Seems that the result_item tha's throwing errors in the tests is, at least according to the code for concurrent.futures.process (https://github.com/python/cpython/blob/98eb40828af97760badfa7b8ff84bd4f7a079839/Lib/concurrent/futures/process.py#L401-L411) supposed to be either ant int or a _ResultItem.

It doesn't seem to be throwing errors at the definition of the _ResultItem class, just at the usage of the result_item argument within the process_result_item method in the _ExecutorManagerThread class

Copy link
Contributor Author

@HunterAP23 HunterAP23 left a comment

Choose a reason for hiding this comment

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

Reviewed and applied

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2021

Diff from mypy_primer, showing the effect of this PR on open source code:

optuna (https://github.com/optuna/optuna.git)
+ optuna/study/_optimize.py:103: error: Incompatible types in assignment (expression has type "AbstractSet[Future[Any]]", variable has type "Set[Future[Any]]")

core (https://github.com/home-assistant/core.git)
+ homeassistant/util/executor.py:67: error: unused "type: ignore" comment
+ homeassistant/util/executor.py:92: error: unused "type: ignore" comment

SinbadCogs (https://github.com/mikeshardmind/SinbadCogs.git)
+ devtools/runner.py:26: error: unused "type: ignore" comment

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2021

Diff from mypy_primer, showing the effect of this PR on open source code:

optuna (https://github.com/optuna/optuna.git)
+ optuna/study/_optimize.py:103: error: Incompatible types in assignment (expression has type "AbstractSet[Future[Any]]", variable has type "Set[Future[Any]]")

SinbadCogs (https://github.com/mikeshardmind/SinbadCogs.git)
+ devtools/runner.py:26: error: unused "type: ignore" comment

core (https://github.com/home-assistant/core.git)
+ homeassistant/util/executor.py:67: error: unused "type: ignore" comment
+ homeassistant/util/executor.py:92: error: unused "type: ignore" comment

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2021

Diff from mypy_primer, showing the effect of this PR on open source code:

optuna (https://github.com/optuna/optuna.git)
+ optuna/study/_optimize.py:103: error: Incompatible types in assignment (expression has type "AbstractSet[Future[Any]]", variable has type "Set[Future[Any]]")

anyio (https://github.com/agronholm/anyio.git)
+ src/anyio/from_thread.py:390: error: Argument 1 to "wait" has incompatible type "Iterable[Future[Any]]"; expected "AbstractSet[Future[<nothing>]]"

SinbadCogs (https://github.com/mikeshardmind/SinbadCogs.git)
+ devtools/runner.py:26: error: unused "type: ignore" comment

core (https://github.com/home-assistant/core.git)
+ homeassistant/util/executor.py:67: error: unused "type: ignore" comment
+ homeassistant/util/executor.py:92: error: unused "type: ignore" comment

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2021

Diff from mypy_primer, showing the effect of this PR on open source code:

core (https://github.com/home-assistant/core.git)
+ homeassistant/util/executor.py:67: error: unused "type: ignore" comment
+ homeassistant/util/executor.py:92: error: unused "type: ignore" comment

SinbadCogs (https://github.com/mikeshardmind/SinbadCogs.git)
+ devtools/runner.py:26: error: unused "type: ignore" comment

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2021

Diff from mypy_primer, showing the effect of this PR on open source code:

optuna (https://github.com/optuna/optuna.git)
+ optuna/study/_optimize.py:103: error: Incompatible types in assignment (expression has type "AbstractSet[Future[Any]]", variable has type "Set[Future[Any]]")

core (https://github.com/home-assistant/core.git)
+ homeassistant/util/executor.py:67: error: unused "type: ignore" comment
+ homeassistant/util/executor.py:92: error: unused "type: ignore" comment

SinbadCogs (https://github.com/mikeshardmind/SinbadCogs.git)
+ devtools/runner.py:26: error: unused "type: ignore" comment

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

The one primer hit seems like an easy and sensible fix. That more than balanced by three now unnecessary type ignores.

@srittau srittau merged commit ffaa0ea into python:master Sep 3, 2021
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

Successfully merging this pull request may close these issues.

4 participants