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

Typing of multiprocessing.Value broken since mypy 0.981 #8799

Closed
prauscher opened this issue Sep 27, 2022 · 9 comments · Fixed by #11576
Closed

Typing of multiprocessing.Value broken since mypy 0.981 #8799

prauscher opened this issue Sep 27, 2022 · 9 comments · Fixed by #11576
Labels
priority: regression Something that worked before doesn't work anymore stubs: false positive Type checkers report false errors

Comments

@prauscher
Copy link
Contributor

prauscher commented Sep 27, 2022

Hello everyone,

after updating to mypy 0.981, i received a mypy error for multiprocessing.Value:

from multiprocessing import Value
import ctypes

mapper = Value(ctypes.c_float, float("nan"))
mapper.value = 2.3  # error: "SynchronizedBase[c_float]" has no attribute "value"

As far as I can tell, the issue is that Value() gives a SynchronizedBase which is not typed to have a value attribute. Synchronized however would offer this attribute, as it is present in the real world.

Am I missing something or should

def Value(self, typecode_or_type: type[_CT], *args: Any, lock: Literal[True] | _LockLike = ...) -> SynchronizedBase[_CT]: ...
be changed to Synchronized instead of SynchronizedBase ?

Update: The issue was not there in 0.971 and is now present in 0.981

@srittau
Copy link
Collaborator

srittau commented Sep 27, 2022

I assume that this was introduced in #8330 by @hauntsaninja. Before that PR, the fourth overload was matched, which just returned Any from Value(), now the second overload is (correctly) matched. The implementation of BaseContext.Value() actually returns the concrete type multiprocessing.sharedctypes.Value, not the (abstract?) SynchronizedBase type. I wonder whether it would make sense to

  • either annotate these methods with the concrete types (and use # type: ignore[override] in sub-classes if necessary); or
  • override these methods in DefaultContext, which is what is eventually used for multiprocessing.Value().

@srittau srittau added stubs: false positive Type checkers report false errors priority: regression Something that worked before doesn't work anymore labels Sep 27, 2022
@Joshix-1
Copy link

Array() has the same problem

"SynchronizedArray[c_char]" has no attribute "value"
"SynchronizedArray[c_char]" has no attribute "raw"

https://docs.python.org/3.10/library/multiprocessing.html#multiprocessing.Array

Note that an array of ctypes.c_char has value and raw attributes which allow one to use it to store and retrieve strings.

@angelo-daumas
Copy link

I've been meaning to address this problem in a PR (as well as other problems in #4266), however, I have a couple of questions:

The main issue is concerning the Synchronized type. At present it is parametrized by the python type, not the ctypes type.

class Synchronized(SynchronizedBase[_SimpleCData[_T]], Generic[_T]):
value: _T

Therefore, the correct usage would be: Synchronized[int], not Synchronized[ctypes.c_int]. However, I feel like the latter would be more correct, because otherwise you're losing information about what underlying ctype is being used. Note that SynchronizedBase is parametrized by the ctypes type, so SynchronizedBase[ctypes.c_int] would be correct. It also seems weird that they would be parametrized differently.

Unfortunately, I don't know how to type hint it so the Synchronized.value property returned the correct type (e.g. an int for something like ctypes.c_int or bool for ctypes.c_bool).

_SCT = TypeVar("_SCT", bound=_SimpleCData[Any])

class Synchronized(Generic[_SCT]):
    value: ?# What could go here? In C++ one could use decltype(self._obj.value), but there's no Py equivalent.

The second question is about the current usage of _SimpleCData and _CData. They are imported in the following file:

from ctypes import _CData, _SimpleCData, c_char

Is it really okay to use these, even though they are prefixed with an underscore? Seems to me like it would mark them as private. There's a related issue about this (#6018). Also, please note that _CData is not actually accessible at runtime from ctypes, it's only available in the stub file. That also seems pretty counterintuitive.

--- TL;DR ---

If the following questions are answered, I can start working on a PR to resolve this issue:

  1. Is Synchronized[int] over Synchronized[ctypes.c_int] really the best option?
  2. If it is not, is there a way to make Synchronized[ctypes.c_int] work while keeping the correct type for Synchronized.value?
  3. Is it really okay to use the private types from ctypes, especially _CData, which is not exported at runtime?

@angelo-daumas
Copy link

angelo-daumas commented Dec 11, 2022

I assume that this was introduced in #8330 by @hauntsaninja. Before that PR, the fourth overload was matched, which just returned Any from Value(), now the second overload is (correctly) matched. The implementation of BaseContext.Value() actually returns the concrete type multiprocessing.sharedctypes.Value, not the (abstract?) SynchronizedBase type. I wonder whether it would make sense to

* either annotate these methods with the concrete types (and use `# type: ignore[override]` in sub-classes if necessary); or

* override these methods in `DefaultContext`, which is what is eventually used for `multiprocessing.Value()`.

As far as I can see, multiprocessing.sharedctypes.Value is a factory function. It's been unfortunately named with an uppercase letter. SynchronizedBase and Synchronized are the types returned. The multiprocessing.Value export is a method of the default context that simply calls multiprocessing.sharedctypes.Value. Please correct me if I'm wrong.

benclifford added a commit to Parsl/parsl that referenced this issue Jan 19, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
benclifford added a commit to Parsl/parsl that referenced this issue Jan 31, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
benclifford added a commit to Parsl/parsl that referenced this issue Feb 1, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
paravoid added a commit to paravoid/ircstream that referenced this issue Feb 20, 2023
typeshed issue revealed by a new mypy version. See:
python/typeshed#8799
benclifford added a commit to Parsl/parsl that referenced this issue Mar 21, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
benclifford added a commit to Parsl/parsl that referenced this issue Mar 30, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
benclifford added a commit to Parsl/parsl that referenced this issue Apr 4, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
benclifford added a commit to Parsl/parsl that referenced this issue Apr 12, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
benclifford added a commit to Parsl/parsl that referenced this issue Apr 24, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
benclifford added a commit to Parsl/parsl that referenced this issue Apr 24, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
benclifford added a commit to Parsl/parsl that referenced this issue Apr 24, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
benclifford added a commit to Parsl/parsl that referenced this issue May 2, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
benclifford added a commit to Parsl/parsl that referenced this issue Jun 23, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
benclifford added a commit to Parsl/parsl that referenced this issue Aug 5, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
@matroscoe
Copy link

This is still an issue with mypy 1.6.0

benclifford added a commit to Parsl/parsl that referenced this issue Oct 26, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
benclifford added a commit to Parsl/parsl that referenced this issue Nov 1, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
benclifford added a commit to Parsl/parsl that referenced this issue Nov 1, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
benclifford added a commit to Parsl/parsl that referenced this issue Nov 1, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
benclifford added a commit to Parsl/parsl that referenced this issue Nov 1, 2023
python/typeshed#8799

that gives this error:

parsl/executors/workqueue/executor.py:674: error: "SynchronizedBase[c_bool]" has no attribute "value"  [attr-defined]


this patch ignores that, until a fix becomes available upstream
@prauscher
Copy link
Contributor Author

As this bug still prevents us from upgrading mypy and using Python 3.12-generic classes: Is there anything I could to to support here? Sadly I'm not too familiar with inner workings of python typing, so I cannot answer any of the questions @angelo-daumas asked, but maybe there is something else to support?

@prauscher
Copy link
Contributor Author

I found a fix that would fix the bug, but am not sure about possible side effects. In https://github.com/python/typeshed/blob/main/stdlib/multiprocessing/context.pyi#L84 (and Line 86) replace SynchronizedBase with Synchronized.
Is there anybody here to check for possible side effects maybe? I'm happy to discuss the issue.

For users, using a manager works too, as the manager uses ValueProxy instead of SynchronizedBase:

from multiprocessing import Value, Manager
import ctypes

manager = Manager()
mapper = manager.Value(ctypes.c_float, float("nan"))
print(mapper.value)

prauscher added a commit to prauscher/typeshed that referenced this issue Mar 11, 2024
could fix python#8799 and python#9898, but request review for side effects.
Without this change, static type checkers would treat `Value(ctypes.c_float, 0.0)` like `ctypes.c_float`, but `Value` offers a field `value` for storage
@f0rmiga
Copy link

f0rmiga commented Mar 12, 2024

@prauscher I'm using the following, with mypy happy:

my_var: Synchronized[bool] = Synchronized(multiprocessing.Value(ctypes.c_bool, True))

I didn't observe any side effects. Since Synchronized is a specialized class from SynchronizedBase, I can't think of any further side effects, but it would indeed be good for more folks to chime in.

@davclark
Copy link

davclark commented May 15, 2024

Pytest complains that Synchronized is not subscriptable, whether you get it direct from multiprocessing, or from typeshed. I haven't dug into why. I'd be curious to hear if anyone is using Pytest with a "subscripted" Synchronized type. I'm NOT using @f0rmiga's approach above, as I'm not going to wrap my already Synchronized Value in another Synchronized instance just to make the type checker happy.

I am using mypy 1.10.0 which I think should support the use of Synchronized here. Same behavior on 3.9 and 3.12.

The simplest way to illustrate the problem is with these two files:

# define_type.py
import ctypes
import time
from multiprocessing import Value
from typing import Any
from multiprocessing.sharedctypes import Synchronized

def a_value() -> Synchronized[ctypes.c_double]:
    val: Synchronized[ctypes.c_double] = Value(ctypes.c_double, lock=False)
    val.value = time.time()  # type: ignore[assignment]
    return val
# test_define_type.py
from define_type import a_value

def test_value():
    v = a_value()
    v.value = 1.2  # type: ignore[assignment]
    assert v is not None

This results in a world where mypy complains (unless told to ignore) about assigning a float to a c_double. But that's fine - the type checker is still helping me by making me check that the assignment makes sense.

running pytest in a directory with these two files results in:

___________________________________________ ERROR collecting test_define_type.py ___________________________________________
test_define_type.py:1: in <module>
    from define_type import a_value
define_type.py:7: in <module>
    def a_value() -> Synchronized[ctypes.c_double]:
E   TypeError: type 'Synchronized' is not subscriptable

Note that mypy will complain about duplicate files if I try including Synchronized from typeshed. So, this could be addressed by mypy somehow giving me a Synchronized type that will work with pytest. But maybe this is pytest's problem. I'll cross post there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: regression Something that worked before doesn't work anymore stubs: false positive Type checkers report false errors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants