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

Recent multiprocessing.Queue change breaks existing annotations and reduces typing precision #4313

Closed
JukkaL opened this issue Jul 6, 2020 · 5 comments · Fixed by #4314
Closed

Comments

@JukkaL
Copy link
Contributor

JukkaL commented Jul 6, 2020

#4289 changed multiprocessing.Queue from a class to a function. This matches the implementation, but this has some possibly unanticipated side effects, as discussed in python/mypy#9100. Here's a summary of the points raised in the linked PR:

  • multiprocessing.Queue is documented as a class: https://docs.python.org/3/library/multiprocessing.html#multiprocessing.Queue

  • multiprocessing.queues.Queue can be used in annotations, but it seems to be undocumented. For a user to annotate code using queues, they need to either read the stubs or the stdlib implementation, which is unfortunate.

  • The return type of multiprocessing.Queue is Queue[Any], so the change loses some typing precision, even if a user can figure out the correct type. Since multiprocessing.queues.Queue is not very discoverable (as it's undocumented), for many new users this may effectively remove the ability to type check queue items.

  • The change breaks existing code that uses multiprocessing.Queue as a type, without an obvious fix.

The above considerations probably explain why multiprocessing.Queue used to be defined as a class, even thought it's not quite true.

I'm not sure what's the best way forward. Here are some ideas:

  • Revert the change to Queue and other similar types (at least for now).
  • Teach type checkers to suggest using multiprocessing.queues.Queue (etc.) in type annotations. However, if these are undocumented, maybe we shouldn't recommend these?
  • Change the return type of multiprocessing.Queue to Queue[T] to preserve type checking precision (this may not work on all type checkers).
@JelleZijlstra
Copy link
Member

Making multiprocessing.Queue an alias for multiprocessing.queues.Queue feels like it might be a good pragmatic solution. (This would be a partial revert of #4289.) This approach could cause a couple of false negatives:

  • Type checkers wouldn't catch code that uses the ctx= argument to the multiprocessing.queues.Queue constructor.
  • Type checkers wouldn't catch code that does isinstance(..., multiprocessing.Queue) or otherwise treats the function as a type object at runtime.

But these problems feel less severe than the problems that @JukkaL points out with the current stubs.

cc @vishalkuo and @hauntsaninja who were involved in the previous PR.

@srittau
Copy link
Collaborator

srittau commented Jul 6, 2020

The ultimate goal should be to have the stubs match the implementation. @JelleZijlstra points out some pitfalls otherwise that a type checker could help catch. As a pragmatic step for now I think Jelle's solution is ok, though. We could also add another stubs-only alias (like _Queue or RealQueue or whatever) to multiprocessing/__init__.pyi and recommend to use this for type annotations of Queue in a comment.

The "clever" implementation of multiprocessing has been a source of constant trouble.

hauntsaninja pushed a commit to hauntsaninja/typeshed that referenced this issue Jul 6, 2020
@hauntsaninja
Copy link
Collaborator

The combination of multiprocessing.queues.Queue being undocumented and multiprocessing.Queue being documented as a class is unfortunate :-( I agree reverting the relevant changes will result in the least surprises for users, opened a PR at #4314

(Note all the changes from #4282 are intact since Array/Value aren't documented as classes. But they're also a little tricky since they return ctypes objects. If people feel the relevant parts of those changes should also be rolled back, this would be the place to discuss that)

@vishalkuo
Copy link
Contributor

Hey all, thanks for flagging this and reverting it. It looks like #4314 reverts my bad change, but let me know if there's anything else you'll need me to resolve around this.

Sorry for all the churn, I tried getting started on this to dip my toes a little deeper into the typed python world but it seems like the multiprocessing module is more complicated than I expected! I'll try to be more careful going forward

@hauntsaninja
Copy link
Collaborator

It's all good, multiprocessing is more complicated than I expected too when I posted #4266 ! In general, as Sebastian said, typing to match the implementation is good — it's just unfortunate that in this case CPython's implementation is complicated and its documentation lies about it :-)

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 a pull request may close this issue.

5 participants