ModbusTcpClient constructor BC break between 3.6.9 and 3.7.0

[rtek]

The ModbusTcpClient had a signature change between 3.6 and 3.7 for the port parameter.

see
https://github.com/pymodbus-dev/pymodbus/blob/v3.6.9/pymodbus/client/tcp.py#L61

 def __init__(
        self,
        host: str,
        port: int = 502,
        framer: Framer = Framer.SOCKET,
        source_address: tuple[str, int] | None = None,
        **kwargs: Any,
    ) -> None:

vs
https://github.com/pymodbus-dev/pymodbus/blob/v3.7.0/pymodbus/client/tcp.py#L61

 def __init__(  # pylint: disable=too-many-arguments
        self,
        host: str,
        framer: FramerType = FramerType.SOCKET,
        port: int = 502,
        name: str = "comm",
        source_address: tuple[str, int] | None = None,
        reconnect_delay: float = 0.1,
        reconnect_delay_max: float = 300,
        timeout: float = 3,
        retries: int = 3,
        on_connect_callback: Callable[[bool], None] | None = None,
    ) -> None:

This results in the following error if using positional arguments for port.

Traceback (most recent call last):
  File "main.py", line 13, in <module>
    client = ModbusTcpClient('localhost', 5502)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "../pymodbus/client/tcp.py", line 158, in __init__
    super().__init__(framer, retries)
  File "../pymodbus/client/base.py", line 190, in __init__
    self.framer: ModbusFramer = FRAMER_NAME_TO_CLASS.get(
                                ^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: 'str' object is not callable

Reverting to 3.6.9 or using kwargs resolves the issue.

[janiversen]

Yes we even documented it, see API_changes.rst

This is not a bug.

[rtek]

Hello - which item referenced this?

https://github.com/pymodbus-dev/pymodbus/blob/dev/API_changes.rst#api-changes-370

Thats the first place I checked and I couldnt find it.

[janiversen]

All the api changes are correctly listed, the individual commits are listed in change_log, you are looking for

• Remove kwargs. (Remove kwargs, part I. #2236, remove kwargs from base pdu #2237)

And this one in api_changes:

• Non defined parameters (kwargs) no longer valid

[janiversen]

We strongly advise against using positional parameters to set non-positional parameters!

The port= have not changed signature, you are just not using it as the signature defines it.

[lafrech]

Got caught by this as well. This could be considered a breaking change needing a major release but I won't complain. It only took me a few minutes to find this issue and fix my code.

We strongly advise against using positional parameters to set non-positional parameters!

If I may suggest, those kwargs could be made keyword-only in the next pymodbus major release (v4).

 def __init__(  # pylint: disable=too-many-arguments
        self,
        host: str,
        *,
        framer: FramerType = FramerType.SOCKET,
        port: int = 502,
        name: str = "comm",
        source_address: tuple[str, int] | None = None,
        reconnect_delay: float = 0.1,
        reconnect_delay_max: float = 300,
        timeout: float = 3,
        retries: int = 3,
        on_connect_callback: Callable[[bool], None] | None = None,
    ) -> None:

Up to you where to put the limit. Can host be positional?

At this point, you may even consider doing this right away since code using positional parameters is broken already.

[janiversen]

First of all, there are no change of signature, port is an optional parameter NOT a positional parameter!

Secondly we have a definition that API changes are only allowed in x.y.0 releases, NOT in x.y.z releases, so going to 3.7.0 means API changes, which are documented in API_changes.rst.

[janiversen]

the positional parameters are NOT broken, again the problem is that you do NOT follow the signature in 3.7 nor in earlier versions.

The signature says "port = 502" meaning it is an optional parameter without a fixed position.

We do not use kwargs ! that was removed in 3.7 especially to make the signatures clearly readable.

[lafrech]

we have a definition that API changes are only allowed in x.y.0 releases

I assumed that would be only in x.0.0, because this is typical semver. But each project is free to decide otherwise. My fault for not reading the docs.

Anyway, I mainly stopped here for the friendly kw-only suggestion, not to rant about the change.

port is an optional parameter NOT a positional parameter!

Then making it keyword-only as I suggest above is the best way to express and enforce it IMHO.

The signature says "port = 502" meaning it is an optional parameter without a fixed position.

Well, I don't think that is the admitted convention. By default in Python, keyword parameters can be supplied positionally. Hence the kw-only PEP.

We do not use kwargs ! that was removed in 3.7 especially to make the signatures clearly readable.

I'm not asking for

 def __init__(self, host: str, **kwargs):
    ...

What I'm talking about is this:

 def __init__(  # pylint: disable=too-many-arguments
        self,
        host: str,
        *,  # <-- args after the star can only be supplied by name, otherwise an exception is raised
        framer: FramerType = FramerType.SOCKET,
        port: int = 502,
        name: str = "comm",
        source_address: tuple[str, int] | None = None,
        reconnect_delay: float = 0.1,
        reconnect_delay_max: float = 300,
        timeout: float = 3,
        retries: int = 3,
        on_connect_callback: Callable[[bool], None] | None = None,
    ) -> None:

Doing this prevents user from supplying args positionally, therefore from being caught by a signature change in the future.

Anyway, just wanted to make my earlier message clearer. I don't mean to argue about this. Code fixed on my side and everything works a treat. Thanks for pymodbus.

[janiversen]

Feature added to the 3.8 list, just have to check if that works across all supported python versions.

thanks for the pointer.

[lafrech]

No pb.

This was introduced much earlier than 3.9. I can't remember but from a quick search it may well be Python 3.0.

[ekscmorente]

Got caught by this as well. This could be considered a breaking change needing a major release but I won't complain. It only took me a few minutes to find this issue and fix my code.

Same here. I totally believe this should have been considered as a breaking change (and therefore a new major version).

+1 here to include keyword-only arguments as explicit keyword arguments using the star definition, otherwise position of the keyword arguments should be considered👍🏻

[janiversen]

Please read the README for the definition of releases !!! it was considered a breaking change and hence not released on 3.6.x but in 3.7.0.

Since it was a small change, and not a demand to rewrite the app it is not a major change......our change from 2.x to 3.x removed the support for python 2, a number of async runners as well as a fundamental changed how the app used the server...THAT was a major change which required significant changes in most apps.

We follow the same release pattern as many (most) of the large projects on GitHub.

The keyword only demand is already on dev, but NOT released because it is a breaking change...it will be release later as 3.8.0

[ekscmorente]

The keyword only demand is already on dev, but NOT released because it is a breaking change...it will be release later as 3.8.0

It's not that much about the release schedule not being clear but more of an issue with pymodbus' versioning not following semver. This may not seem like an issue until you have automations and/or dependency management tools... I guess either following semver or at least stating a warning that you're not following it would help a lot 😄

[janiversen]

If you want a warning, then feel free to submit a pull request, this is open source where you are expected to help.

We do not document what we do not follow (that is quite impossible) but we document extensively what we follow, so reading the documentation (and at the very least the release notes is a good idea).

[janiversen]

Apart from that semver rule 7 says:
"Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backward compatible functionality is introduced to the public API. It MUST be incremented if any public API functionality is marked as deprecated. It MAY be incremented if substantial new functionality or improvements are introduced within the private code. It MAY include patch level changes. Patch version MUST be reset to 0 when minor version is incremented."

version 3.7 is backwards compatible 3.6, IF you use the signature as defined: that is use keyword and not replace a keyword parameter with a positional parameter. If you do not follow the signature some calls will be ok others not, e.g. a lot of the "int" signatures also accept other format like "bytes" and "float", but that is due to the fact that python often convert automatically.