Fix typing for reactive server #1371

alexrudd2 · 2023-02-20T23:05:56Z

Solves the following mypy errors:

pymodbus/server/reactive/main.py:441: error: Argument 1 to "ReactiveModbusSlaveContext" has incompatible type "**Dict[Any, Dict[Any, Any]]"; expected "BaseModbusDataBlock" [arg-type]
pymodbus/server/reactive/main.py:450: error: Incompatible types in assignment (expression has type "ReactiveModbusSlaveContext", variable has type "Dict[int, ReactiveModbusSlaveContext]") [assignment]

Co-Authored-By: jan iversen <[email protected]>

janiversen

Some comments.

janiversen · 2023-02-21T07:45:42Z

pymodbus/datastore/store.py

 #  Datablock Storage
 # ---------------------------------------------------------------------------#
-class BaseModbusDataBlock:
+class BaseModbusDataBlock(dict):


Why does it inherit from a dict, it really have nothing to do with a dict.

And in most datastores the values are not stored in a dict.

Inhering from dict would mean the following is a legal use:

block = BaseModbusDataBlock() response = block["address"]

But that is not the use case, and never will be, since we never just get address, we always request address, count (using get/set/validate).

The block dict is passed to the constructor for ReactiveModbusslaveContext using ** unpacking.

pymodbus/pymodbus/server/reactive/main.py

Lines 440 to 446 in 031c457

slave_context = ReactiveModbusSlaveContext(

**block,

randomize=randomize,

change_rate=change_rate,

zero_mode=True,

**data_block_settings,

)

That constructor takes several BaseModbusDataBlocks as keyword arguments.

pymodbus/pymodbus/server/reactive/main.py

Lines 103 to 116 in 031c457

class ReactiveModbusSlaveContext(ModbusSlaveContext):

"""Reactive Modbus slave context"""

def __init__(

self,

discrete_inputs: BaseModbusDataBlock = None,

coils: BaseModbusDataBlock = None,

input_registers: BaseModbusDataBlock = None,

holding_registers: BaseModbusDataBlock = None,

zero_mode: bool = False,

randomize: int = 0,

change_rate: int = 0,

**kwargs,

):

This means that block must be something like Dict[str, BaseModbusDataBlock]

So any block[item] must also be BaseModbusDataBlocks, which means the two ways of creating an item must follow this:

pymodbus/pymodbus/server/reactive/main.py

Lines 432 to 438 in 031c457

block[modbus_entity] = {

add: val

for add in sorted(address_map)

for val in default_values

}

else:

block[modbus_entity] = db(start_address, default_values)

(1) block[modbus_entity] = {... } fails because it's too broad (a dict).
(2)block[modbus_entity] = db(...) fails for because db is a too broad (also a dict). If I type db to BaseModbusDataBlock, it cannot take the two arguments, anyways.

....somewhere around here I gave up, and made BaseModbusDataBlock(dict). :)
Typing is very difficult; I will try again.

You are again mixing issues....you talk about how the datastore is defined (init) and then by adding dict, you redefine the usage to be as a dict, which is wrong.

Please accept the 3 views of the datastore, that might help you quite a bit.

janiversen · 2023-02-21T07:49:22Z

pymodbus/datastore/store.py

-        self.values = [  # pylint: disable=attribute-defined-outside-init
-            self.default_value
-        ] * count
+        self.values = [self.default_value] * count


This does not seem right, and you missed the previous disable !

The problem is that the base class misses an init where the variables should be defined.

In this case I followed pylint blindly. It warned me the disable was unnecessary. I will revisit after rethinking BaseModbusDataBlock(dict)

janiversen · 2023-02-21T07:50:05Z

pymodbus/server/reactive/main.py

        slaves = {}
        for i in unit:
-            block = {}
+            block = BaseModbusDataBlock()


I think this is wrong, but I have to look deeper into it.

On further analysis (see above), yes it's wrong. It should be Dict[str, BaseModbusDataBlock]

No it should be "{}" or "dict", it is NOT a BaseModbusDataBlock, it is the setup to define a BaseModbusDataBlock, unless I have misunderstood the code.

You are again mixing issues....you talk about how the datastore is defined (init) and then by adding dict, you redefine the usage to be as a dict, which is wrong.
No it should be "{}" or "dict", it is NOT a BaseModbusDataBlock, it is the setup to define a BaseModbusDataBlock, unless I have misunderstood the code.

pymodbus/server/reactive/main.py:441: error: Argument 1 to "ReactiveModbusSlaveContext" has incompatible type "**Dict[Any, Dict[Any, Any]]"; expected "BaseModbusDataBlock" [arg-type]

mypy is very strict, please try running it and you will see why I think in these ways. It does not care about the intended use of the code, only about how broad/narrow types are. If a variable is defined narrowly in some downstream code, then all previous users of that variable must be as narrow. They cannot be broader (which is what the error is here). This includes the initialization; there is nothing special about it.

block: Dict[str, BaseModbusDataBlock] != block: BaseModbusDataBlock. It means a dict with keys that are str and values that are BaseModbusDataBlock.

If block is permitted to be any arbitrary dict, then (I think) mypy is concerned that some code may do something stupid. For instance, block['coils'] = 3.

BaseModbusDataBlock(dict) was a mistake because it was too broad. Instead I need to find a way to make the other definitions more narrow.

===

Please accept the 3 views of the datastore, that might help you quite a bit.

Yes, it's true. Thank you for the explanations, I am slowly learning the intent of the code. It's difficult to serve two masters, you and mypy. :)

[Argument 1 to React](error: Argument 1 to "ReactiveModbusSlaveContext") does not mean that ModbusSlaveContext should be a dict !

I am aware that mypy likes strict definition and I have NOTHING against that, but block is NOT a BaseModbusDataBlock, it is used to define one.

BaseModbusDataBlock do not inherit from anything ! that is a wrong idea. You need to add more strict definitions where data are defined for the DataBlock, which is in Init.

Be aware ReactiveModbusSlaveContext is yet another context version, look in directory datastore and you will find more. Being inherited means that it:

can expand the base init to allow new type of definitions

has its own internal way of storing things.
BUT get/set/validate must remain unchanged, or it would break the server.

[Argument 1 to React](error: Argument 1 to "ReactiveModbusSlaveContext") does not mean that ModbusSlaveContext should be a dict !

When did I say this? I explained how block, which is a dict, is unpacked using ** into (a subclass of) ModbusSlaveContext.

block is NOT a BaseModbusDataBlock, it is used to define one.

I said that block is currently a dict, and the members of block (such as coils) must be BaseModbusDataBlock by the time they are used to initialize ReactiveModbusSlaveContext, which is typed to require e.g. coils: BaseModbusDataBlock (30f7166).

BaseModbusDataBlock do not inherit from anything ! that is a wrong idea.

I am not arguing for BaseModbusDataBlock(dict), and have agreed many times it's wrong. It's why I changed the PR to draft status. I'm also not arguing for ModbusSlaveContext(dict). I will revert the commit so you can stop focusing on telling me this, and perhaps try to work on a proper solution.

pymodbus/pymodbus/server/reactive/main.py

Lines 440 to 446 in 031c457

slave_context = ReactiveModbusSlaveContext(

**block,

randomize=randomize,

change_rate=change_rate,

zero_mode=True,

**data_block_settings,

)

I'm arguing that the unpacking of block into the the arguments for [Reactive]ModbusSlaveContext means that block should probably be Dict[str, BaseModbusDataBlock].

block: Dict[str, BaseModbusDataBlock] = { "coils": BaseModbusDataBlock(), "discrete_inputs" : BaseModbusDataBlock(), [...] } slave_context = ReactiveModbusSlaveContext( **block, [...]) i.e. slave_context = ReactiveModbusSlaveContext( coils = BaseModbusDataBlock(), discrete_inputs=BaseModbusDataBlock() )

I think it is better I let you work, and will review when it is ready.

May I polity recommend you look at our examples to see how we use Context. BaseModbusDataBlock is a Base not a type.

/s/BaseModbusDataBlock/Type[BaseModbusDatablock]

Perhaps that was our confusion. It gets annoying when sometimes you need Type[] (for arguments), and sometimes you need class instances (in return values).
https://adamj.eu/tech/2021/05/16/python-type-hints-return-class-not-instance/.

Thank you for your patience with me. Typing is difficult to learn in complex cases.

No problem, we both pursue the same goal, a better library.

Thinking about Type[], but without knowing it, could this be a solution to mixin.py ?

Thinking about Type[], but without knowing it, could this be a solution to mixin.py ?

Sadly, no. The "abstract" execute has a broader type, and the individual callers expect a narrower one.

This reverts commit d07b8bc.

alexrudd2 · 2023-02-21T19:56:30Z

closing in favor of #1381

Initalize block as the complete type rather than an empty dict

d07b8bc

Co-Authored-By: jan iversen <[email protected]>

alexrudd2 marked this pull request as draft February 20, 2023 23:06

Use a 1-item dict to keep consistent typing

2ebdbc2

Co-Authored-By: jan iversen <[email protected]>

alexrudd2 marked this pull request as ready for review February 21, 2023 01:25

janiversen requested changes Feb 21, 2023

View reviewed changes

alexrudd2 marked this pull request as draft February 21, 2023 15:48

Revert "Initalize block as the complete type rather than an empty dict"

129942f

This reverts commit d07b8bc.

alexrudd2 mentioned this pull request Feb 21, 2023

Fix mypy errors in reactive server #1381

Merged

alexrudd2 closed this Feb 21, 2023

alexrudd2 deleted the mypy-reactive branch February 21, 2023 20:10

github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023

	slave_context = ReactiveModbusSlaveContext(
	**block,
	randomize=randomize,
	change_rate=change_rate,
	zero_mode=True,
	**data_block_settings,
	)

	class ReactiveModbusSlaveContext(ModbusSlaveContext):
	"""Reactive Modbus slave context"""

	def __init__(
	self,
	discrete_inputs: BaseModbusDataBlock = None,
	coils: BaseModbusDataBlock = None,
	input_registers: BaseModbusDataBlock = None,
	holding_registers: BaseModbusDataBlock = None,
	zero_mode: bool = False,
	randomize: int = 0,
	change_rate: int = 0,
	**kwargs,
	):

	block[modbus_entity] = {
	add: val
	for add in sorted(address_map)
	for val in default_values
	}
	else:
	block[modbus_entity] = db(start_address, default_values)

Fix typing for reactive server #1371

Fix typing for reactive server #1371

Uh oh!

Conversation

alexrudd2 commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janiversen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexrudd2 Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexrudd2 commented Feb 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexrudd2 commented Feb 20, 2023 •

edited

Loading

alexrudd2 Feb 21, 2023 •

edited

Loading