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

[multiprocessing] fixes some stubtest cases for Value and Array #4282

Merged
merged 6 commits into from
Jun 27, 2020

Conversation

vishalkuo
Copy link
Contributor

@vishalkuo vishalkuo commented Jun 26, 2020

I wanted to get my toes a little more wet into stubtests so I tried to take a stab at #4266. I believe this fixes two test cases:

multiprocessing.Array is not a type
multiprocessing.Value is not a type

I'm still not sure I understand why this works. The only reason I arrived at this solution is by following the pattern for line 44 defined in the same file which uses a function for Semaphore despite it also being a class according to the docs. Does this have to do with mp having a dynamic nature? Is this the correct solution?

Test Plan:

(mypy) [~/projects/typeshed] ((HEAD detached at d4a2cf3d)) $ python3.8 -m mypy.stubtest --custom-typeshed-dir . multiprocessing --concise   | grep "\.Value\|\.Array"
multiprocessing.Array is not a type
multiprocessing.Value is not a type
(mypy) [~/projects/typeshed] ((HEAD detached at d4a2cf3d)) $ git co fix 
Previous HEAD position was d4a2cf3d [redis] adds additional sorted set type information (#4278)
Switched to branch 'fix'
(mypy) [~/projects/typeshed] (fix) $ python3.8 -m mypy.stubtest --custom-typeshed-dir . multiprocessing --concise   | grep "\.Value\|\.Array"
(mypy) [~/projects/typeshed] (fix) $ 

@vishalkuo
Copy link
Contributor Author

Second note is that Value and Array actually exist inside a separate sharedctypes.py file in the actual lib: https://github.com/python/cpython/blob/e42b705188271da108de42b55d9344642170aa2b/Lib/multiprocessing/sharedctypes.py#L19 should we move these two there?

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jun 27, 2020

Ooo, there are some easy fixes in that issue, but this is a tricky one. There are several improvements to be made to the stubs here, that are separate from stubtest's actual (but minor to the end user) complaint. I can explain a little more (and forgive me if I over-explain):

So the core issue is that these class-looking things in multiprocessing are actually functions returning ctypes objects.

In [4]: multiprocessing.Array                                                                                                                              
Out[4]: <bound method BaseContext.Array of <multiprocessing.context.DefaultContext object at 0x10527d5d0>>

Why does stubtest complain here? stubtest sees that the stubs define a class, and so tries to confirm that the runtime object is an instance of type. As seen, the runtime object is a method, hence error:
https://github.com/python/mypy/blob/8219564d365ba29dda8c4383594d9db91f26feec/mypy/stubtest.py#L235
This isn't the worst kind of error, since having a class in the stubs shouldn't give you false positives, but it might give you a false negative if you try to do things that would only work if multiprocessing.Array was actually a class, e.g. this code would pass mypy:

In [7]: x = multiprocessing.Array('i', range(10))                                                                                                          

In [8]: x                                                                                                                                                  
Out[8]: <SynchronizedArray wrapper for <multiprocessing.sharedctypes.c_int_Array_10 object at 0x1053a2ef0>>

In [9]: isinstance(x, multiprocessing.Array)                                                                                                               
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-9-f4526957538c> in <module>
----> 1 isinstance(x, multiprocessing.Array)

TypeError: isinstance() arg 2 must be a type or tuple of types

What should we do? I would do the following:

  1. As you say, create a stub for sharedctypes.
  2. In that, we can put classes, or if we want to be a little more accurate, we could have functions that return instances of private classes, as you've done in this PR
  3. We should update the methods in context to return those types:
    # TODO: typecode_or_type param is a ctype with a base class of _SimpleCData or array.typecode Need to figure out
  4. We could duplicate what we just did in __init__.py, but it might make sense to mirror the runtime structure a little more. In which case we could add _default_context: DefaultContext to multiprocessing.context and then add things like Array = _default_context.Array to __init__.py to mimic what CPython does.

This isn't all or nothing either, even just making a sharedctypes and not doing the rest would improve what we have today! This PR is an improvement over today as well! 🙂
I think this would qualify as more than just toe dipping 😄, the easier parts of #4266 are probably complaints about missing arguments.

Also this turned out to be a hefty wall of text, I hope it makes sense!!

@vishalkuo
Copy link
Contributor Author

Thanks for the detailed feedback, @hauntsaninja. I've added what I think is the right way to approach this.

  1. introduce sharedctypes.pyi. Note that this only adds Array and Value, and I might add the next ones in future diffs. As such, this actually adds more stubtest errors for now. Let me know if that's not acceptable.
(mypy) (⎈ |development:home)[~/projects/typeshed] (fix) $ python3.8 -m mypy.stubtest --custom-typeshed-dir . multiprocessing --concise    | grep sharedctypes
multiprocessing.sharedctypes.RawArray is not present in stub
multiprocessing.sharedctypes.RawValue is not present in stub
multiprocessing.sharedctypes.copy is not present in stub
multiprocessing.sharedctypes.synchronized is not present in stub
  1. Have Array and Value in BaseContext return these new private classes from sharedctypes
  2. Have __init__.pyi use a default context

One thing I'm not sure about - it seems like Array and Value actually take a ctx argument seen here. My best guess is it's a t.Optional[BaseContext] but even the docs don't recognize this as an argument. I've left this untyped for now. Let me know if that's incorrect.

@vishalkuo
Copy link
Contributor Author

Ah I derped on 3, the context is from the sharedctypes method and not the context.pyi method: https://github.com/python/cpython/blob/2e0a920e9eb540654c0bb2298143b00637dc5961/Lib/multiprocessing/context.py#L138. I'll update my diff to make it an Optional BaseContext

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jun 27, 2020

This looks great! It's too bad pytype doesn't seem to understand the bound method :-( (pytype is often less sophisticated than mypy). I guess we can just duplicate the definition from context.pyi minus self.

@vishalkuo
Copy link
Contributor Author

Heh, I just came across

# Note: it's somewhat unfortunate that we have to copy the function signatures.
# It would be nice if we could partially reduce the redundancy by doing something
# like the following:
#
# _screen: Screen
# clear = _screen.clear
#
# However, it seems pytype does not support this type of syntax in pyi files.
which seems to be the exact same problem

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.

Nice find, hopefully this gets everything green!

@vishalkuo
Copy link
Contributor Author

Sweet! Thanks for the help

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.

2 participants