Improve sync and async type checks instead of Union[sync | async]

[laundmo]

Versions

• Python: 3.10
• OS: irrelevant
• Pymodbus: 3.6.4
• Modbus Hardware (if used): irrelevant

Pymodbus Specific

irrelevant

Description

The PR #1842 prematurely merged a set of typhints in a attempt to allow type-checking async code, effectively breaking both sync and async usecases.

The issue with this was discussed inside the PRs later messages, but as it was locked (why? why not just leave it open if theres need to follow up?) i am making this issue to request reverting that PR, while considering a better set of typehints.

Code and Logs

irrelevant

[laundmo]

it seems that since #1878 a possiblity would be to type-hint in the concrete sync and async clients, instead of the generic mixin class

[alexrudd2]

it was locked (why? why not just leave it open if there's need to follow up?)

It wasn't manually locked. Rather an automatic GitHub Action to prevent comments on old issues, in an effort to push people to using newer code and the Discussions feature. It's a tradeoff. 🤷

a possibility would be to type-hint in the concrete sync and async clients, instead of the generic mixin class

I may not know how to attempt that myself, but if you give it a try I may be able to help.

[janiversen]

If I may correct #1842, was not prematurely merged ! We knew it was not a perfect solution, but only a step in the direction and I for one, saw it as the better option to merge it instead of continuing waiting for the perfect solution.

I am all ears for a solution (like one in the clients) that do NOT duplicate code ! It is correct that we now have 2 base clients, but copying mixin into the 2 base classes, will double the tests and maintenance, and are sure to provide problems down the road (someone changes e.g. the sync call but not the async call).

With my experience from other projects, the most robust solution would be to have a mixin_generator.py, that generates mixin_async.py and mixin_sync.py.

Seen from my life as maintainer I can live with the very old solution, no types at all, but I highly respect the work of @alexrudd2 and others to make the library type checked and agree it makes the library better.

[janiversen]

Just checked to be sure, mypy controls all our examples as well as all our test files, so it cannot be totally broken, otherwise we would have had mypy issues.

What I have learned is that both mypy and ruff are very sensitive to the options used, so please be sure you use the options as we do...we know for a fact that mypy will complain heavily with some options active.

[laundmo]

t.py

from pymodbus.client import AsyncModbusTcpClient, ModbusTcpClient


def get_coil1(client: ModbusTcpClient) -> bool:
    return client.read_coils(1, 1, 1).bits[0]


async def get_coil2(client: AsyncModbusTcpClient) -> bool:
    a = await client.read_coils(1, 1, 1)
    return a.bits[0]

this simple code, with the tool.mypy section from your pyproject.toml, produces these errors:

t.py:5: error: Item "Awaitable[ModbusResponse]" of "ModbusResponse | Awaitable[ModbusResponse]" has no attribute "bits"  [union-attr]
t.py:5: note: Maybe you forgot to use "await"?
t.py:9: error: Incompatible types in "await" (actual type "ModbusResponse | Awaitable[ModbusResponse]", expected type "Awaitable[Any]")  [misc]
Found 2 errors in 1 file (checked 1 source file)

as you can see, both the sync and async cases produce a error here.

[janiversen]

What can I say:

    print("connect to server")
    await client.connect()
    # test client is connected
    assert client.connected

    print("get and verify data")
    try:
        # See all calls in client_calls.py
        rr = await client.read_coils(1, 1, slave=1)
    except ModbusException as exc:
        print(f"Received ModbusException({exc}) from library")
        client.close()
        return

in simple_async_client.py, passes mypy

(pymodbus) jan@piso_macmini pymodbus % mypy examples/simple_async_client.py 
Success: no issues found in 1 source file
(pymodbus) jan@piso_macmini pymodbus % 

[janiversen]

same goes for the sync client (simple_sync_client.py):

    print("get and verify data")
    try:
        rr = client.read_coils(1, 1, slave=1)
    except ModbusException as exc:
        print(f"Received ModbusException({exc}) from library")
        client.close()
        return

mypy run:

(pymodbus) jan@piso_macmini pymodbus % mypy examples/simple_sync_client.py 
Success: no issues found in 1 source file

[laundmo]

thats likely because you are not type-hinting the client type, so it figured out something that way. try passing the client to a typehinted function as i have done.

[janiversen]

The example works, that is enough for me....we have never promised full type hinting, someday we hopefully will.

Reverting the other PR as you suggest, do not solve the problem, just changes it, so it is not a good solution.

Feel free to submit a PR with a proper solution and we will review it in a positive manner.

[alexrudd2]

The example works, that is enough for me....we have never promised full type hinting, someday we hopefully will.

The type hints are indeed incorrect, they're just not being checked.

Use mypy --check-untyped-defs to see this.

Feel free to submit a PR with a proper solution

I'm sorry I don't have one at this time.

[laundmo]

the PR has actively broken type hints in either case, which is generally cosidered worse than none. i have opted to downgrade

[janiversen]

OK, but you must admit that reverting do not solve the problem with type hinting, so with or without this PR there are problems !

Downgrading is of course one way, helping to solve the problem properly is another, and inline with the idea of open source.

[laundmo]

to be honest, a proper solution would likely see the removal of py.typed until full typechecking is possible, as that is the marker for typecheckers that a package is fully type checkable

[janiversen]

I cannot comment on that, I have no idea of whether or not py.typed mark "fully" or "partly" checkable.

I know that pymodbus API have never been fully checkable.

@alexrudd2 is our local mypy specialist, so I suppose he can comment on our use of py.typed.

[alexrudd2]

to be honest, a proper solution would likely see the removal of py.typed until full typechecking is possible, as that is the marker for typecheckers that a package is fully type checkable

A fair argument. On the other hand, PEP 561 never says "fully: :) As the primary advocate of type checks here, doing things incrementally has been useful. I believe it's possible to do something like below in your calling code, no?

[mypy-pymodbus.*]
check_untyped_defs = False

[alexrudd2]

it seems that since #1878 a possiblity would be to type-hint in the concrete sync and async clients, instead of the generic mixin class

Let's keep the issue open for now in order to investigate this suggestion.

(unfortunately I no longer work for a company using pymodbus so this is all for fun in my spare time)

[janiversen]

@alexrudd2 welcome to the club ! ALL my maintenance is fun spare time.

[laundmo]

PEP 561 never says "fully"

its heavily implied with language and the fact you can write "partial" in py.typed when used for type stubs.

I believe it's possible to do something like below in your calling code, no?

[mypy-pymodbus.*]
check_untyped_defs = False

alas, it is not, because i use pylance/pyright (vscode python language server) and not mypy (unless to check whether a issue is localized to pyright or also applies to mypy)

[alexrudd2]

alas, it is not, because i use pylance/pyright (vscode python language server) and not mypy (unless to check whether a issue is localized to pyright or also applies to mypy)

Would # pyright: basic help? (trying pyright is on my to-do list but I have no experience with it)

[janiversen]

Let's be a little careful here. We use "mypy" to control the source, we do not currently support pyright/pylance !!

pyright / pylance might or might not work, like many other checkers, but it is currently outside the scope of this project.

So if "check_untyped_defs = False" works, then all we miss is to document it.

[laundmo]

i never said you should support pyright, i just explained why the proposed solution won't work for me.

[laundmo]

i have re-run mypy with --check-untyped-defs and it does not solve it.

the core issue is that both ModbusTcpClient and AsyncModbusTcpClient (for example, i know there are other clients) are type-hinted with ModbusResponse | Awaitable[ModbusResponse]

this type-hint says "this function can return either" without any further clarification that a certain type of client can only return one or the other.
in that sense, it is always wrong as no sync client can return a awaitable and no async client can return the result directly.
checking untyped definitions obviously cannot alleviate this - the typehints would need to.

here is a minimal example of the issue, and a solution within this minimal example:

from typing import Awaitable

class BaseClient:
    def execute(self) -> int | Awaitable[int]:
        return 1

class SyncClient(BaseClient):
    pass

class AsyncClient(BaseClient):
    pass

def use_sync(a: SyncClient) -> int:
    return a.execute()

a solution using Generics

from typing import Awaitable, Generic, TypeVar

T = TypeVar("T")

class BaseClient(Generic[T]):
    def execute(self) -> T:
        raise NotImplementedError("Specific client needs to implement execute")

class SyncClient(BaseClient[int]):
    pass

class AsyncClient(BaseClient[Awaitable[int]]):
    pass

def use_sync(a: SyncClient) -> int:
    return a.execute()

This way, each specific client can specify the return type it would require once. This works well in this case since theres only ever one return type.

It does require that the mixin raise errors instead of return defaults, though i hope nothing relies on the mixins default return values.

[janiversen]

That is a very elegant solution!!

The limitation of raising errors is not an issue as far as I can see.

Looking forward to see a pull request.

[laundmo]

made a PR, works for me. exactly what was discussed here.

[janiversen]

Just reviewed the PR, and in general looks very elegant. From my side it can be merged once the review comments are addressed.