Consistent ImportError messages #1827

alexrudd2 · 2023-10-17T02:08:05Z

janiversen · 2023-10-17T06:33:11Z

pymodbus/client/serial.py

    import serial
+except ImportError:


This is wrong, because it demands users to have the dependency installed.

The existing version, allows the user not to have the dependency installed, but of course he/she is then not allowed to use these methods.

We do NOT want to demand users to install e.g. pyserial / aiohttp if they are going to use serial communication / the simulator.

what I did earlier to avoid followup errors was to have

except ImportError: serial = None

We do NOT want to demand users to install e.g. pyserial / aiohttp if they are going to use serial communication / the simulator.

I'm confused. What use is importing the serial client without pyserial, or importing the HTTP simulator without aiohttp? They will not run, and fail with some confusing error message.

Are you concerned about importing pymodbus to use another part? (for instance, the TCP client as in #1128) I checked and this is not a problem.

>>> import serial Traceback (most recent call last): File "<stdin>", line 1, in <module> ModuleNotFoundError: No module named 'serial' >>> from pymodbus.client import AsyncModbusTcpClient >>>

Last year pymodbus did something like import *, but now after your refactors it does not. So I'm not sure how the ImportError would be triggered when it shouldn't be.

Because the user does "import pymodbus" and that scans all py files, and it then breaks down.

Try to uninstall pyserial and try for yourself, I had the problem about a month ago. aiohttp was never the problem, but pyserial have caused a couple of issues.

See this:

(pymodbus) jan@MacMini examples % pip uninstall pyserial ... (pymodbus) jan@MacMini examples % ./client_async.py Traceback (most recent call last): File "/Volumes/DiskHomes/home_jan/repos/pymodbus/pymodbus/client/serial.py", line 17, in <module> import serial ModuleNotFoundError: No module named 'serial' During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/Volumes/DiskHomes/home_jan/repos/pymodbus/examples/./client_async.py", line 35, in <module> from pymodbus.client import ( File "/Volumes/DiskHomes/home_jan/repos/pymodbus/pymodbus/client/__init__.py", line 16, in <module> from pymodbus.client.serial import AsyncModbusSerialClient, ModbusSerialClient File "/Volumes/DiskHomes/home_jan/repos/pymodbus/pymodbus/client/serial.py", line 19, in <module> raise ImportError( # pylint: disable=raise-missing-from ImportError: Serial client requires pyserial Please install with "pip install pyserial" and try again. (pymodbus) jan@MacMini examples %

client_async without parameters uses a tcp client.

My test was done with this PR.

Because the user does "import pymodbus" and that scans all py files, and it then breaks down. [...]
Try to uninstall pyserial and try for yourself, I had the problem about a month ago.

Yes, I did :). It's fine

(pymodbus) jan@MacMini examples % ./client_async.py Traceback (most recent call last): File "/Volumes/DiskHomes/home_jan/repos/pymodbus/pymodbus/client/serial.py", line 17, in <module> import serial ModuleNotFoundError: No module named 'serial'

OK, now I understand what I missed. The examples fail, not all of pymodbus. I'll look to see if that can be changed.

alexrudd2 · 2023-10-17T16:08:07Z

changes that makes life harder for users of the library or complicates the code just to please mypy are at the very least something that should be discussed.

Yes, that is my intention as well!

Depending on how it's called, dev produces one of these error messages.

>       if not web:
E       NameError: name 'web' is not defined

>>> from pymodbus.server.simulator import ModbusSimulatorServer
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: cannot import name 'ModbusSimulatorServer' from 'pymodbus.server.simulator' (/Users/a.ruddick/Documents/github/pymodbus/pymodbus/server/simulator/__init__.py)

With this PR, the error message is below. I find this much more helpful.

<traceback>
ImportError: Simulator server requires aiohttp. Please install with "pip install aiohttp" and try again.

janiversen · 2023-10-17T16:28:30Z

Well the code as it is without this PR, do not have problem, nor did it have problems with the "web".

But the error message you get is exactly what we try to avoid by making a conditional import. In reality, apart from the error message, there are no reason for your try/except.

Clearly the app import pymodbus.server.simulator or instantiate ModbusSerialClient, the dependency must be installed....but if the app does not of that the code should work...see the test I just made above.

alexrudd2 · 2023-10-17T16:30:10Z

In reality, apart from the error message, there are no reason for your try/except.

Yes, I agree. The entire point (instead of contextlib.suppress) is for a more helpful error message, so people do not try to e.g. pip install serial and get confused.

janiversen · 2023-10-17T16:32:31Z

In reality, apart from the error message, there are no reason for your try/except.

Yes, I agree. The entire point (instead of contextlib.suppress) is for a more helpful error message, so people do not try to e.g. pip install serial and get confused.

You miss the point, it is a conditional import, so all the users that do not use simulator and/or serial do not need to install these libraries...and as you might already have seen from my test above, uninstalling pyserial and running client_async.py caused problems.

janiversen · 2023-10-17T16:34:04Z

The solution we have currently on dev allows:

$ pip uninstall pyserial
$ ./client_async.py --comm tcp

This PR should not change that.

alexrudd2 · 2023-10-17T16:36:15Z

The solution we have currently on dev allows:
$ pip uninstall pyserial
$ ./client_async.py --comm tcp
This PR should not change that.

Yes, understood. I think the answer is to use conditional importing (and/or error suppression) in the examples as well.

ll the users that do not use simulator and/or serial do not need to install these libraries

s/all the users/all the users of examples. (Users that directly import the functions are fine).

janiversen · 2023-10-17T16:39:35Z

The solution we have currently on dev allows:
$ pip uninstall pyserial
$ ./client_async.py --comm tcp
This PR should not change that.
Yes, understood. I think the answer is to use conditional importing (and/or error suppression) in the examples as well.

NO because the examples are building block of applications, and we do not want to force applications to use conditional imports.

Apart from the fact that I would not know which import should be conditional...because the application import pymodbus, pymodbus.client etc....the application do not even know (in theory) that we import pysierlal and other dependencies.

alexrudd2 · 2023-10-17T16:50:37Z

Can I ask you to slow down a bit, please? 😃

because the application import pymodbus, pymodbus.client

Yes, both of these are fine.

>>> import serial
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'serial'
>>> from pymodbus.client import AsyncModbusTcpClient
>>>

Only the examples are problematic, because they import everything (even if unneeded). It is here where I propose to suppress the unused imports.

pymodbus/examples/client_async.py

Lines 37 to 42 in 4ddb9b1

    
           from pymodbus.client import ( 
        
               AsyncModbusSerialClient, 
        
               AsyncModbusTcpClient, 
        
               AsyncModbusTlsClient, 
        
               AsyncModbusUdpClient, 
        
           )

janiversen · 2023-10-17T16:59:24Z

And I politely disagree 😃 The application should be allowed to import everything, but not to instantiate the classes.

I am not sure we still have it, but earlier you would get an error message if you did client = AsyncModbusSerialClient() and did not have the library installed.

Please do not forget that many users copy our examples (that is sort of the idea) so if we introduce conditionals in the examples it grows into the applications.

I really do not understand what we are discussing....this works nicely on dev, so why change it to something less handy ?

alexrudd2 · 2023-10-17T17:13:20Z

And I politely disagree 😃 The application should be allowed to import everything, but not to instantiate the classes.

OK, perhaps it's best to check at time of use rather than time of import.

I am not sure we still have it, but earlier you would get an error message if you did client = AsyncModbusSerialClient() and did not have the library installed.

No, it fails silently now.

I really do not understand what we are discussing....this works nicely on dev, so why change it to something less handy ?

Well, there are multiple cases that are inconsistent.

Running the reactive simulator without aiohttp gives a clear warning and does a sys.exit(1).
Running the HTTP simulator with aiohttp gives a confusing NameError

 if not web:
E       NameError: name 'web' is not defined

Running the client(s) or examples without pyserial gives a NameError, which is OK but could be better (suggesting pyserial not serial)

I am trying to make them all the same, and helpful. (Also avoid the wrath of strict mypy 😆 )

janiversen · 2023-10-17T17:21:14Z

Sorry if the error message dropped out (aiohttp in simulator probably never had it, I am guilty in not being consistent) !

I am all in favour of having a "common" error message if a dependency is missing ...

I think the easiest (apart from mypy) is something like:

try
   import x
except:
  x = None

And then in the relevant base class init():

   if not x:
     nice error message
     exit(-1)

I have no idea how that works with mypy, but it would work well with users.

alexrudd2 · 2023-10-17T17:26:07Z

Sorry if the error message dropped out (aiohttp in simulator probably never had it, I am guilty in not being consistent) !

Actually, you did include it and it used to work (but was broken by the contextlib.suppress change)

pymodbus/pymodbus/server/simulator/http_server.py

Lines 120 to 121 in 4ddb9b1

    
           if not web: 
        
               raise RuntimeError("aiohttp not installed!")

How about this?

try:
   import x
   x_missing = True
except:
   x_missing = False

__init__():
    if x_missing:
       nice error message and exit

alexrudd2 · 2023-10-17T17:27:34Z

Also, sys.exit(1) is very ugly with pytest. (It kills the whole test collection runner if the import fails).

This is why I re-raised an ImportError for the nice error message instead.

janiversen · 2023-10-17T17:34:10Z

Your suggestion is a good compromise, with a small change, because I agree that sys.exit is ugly, how about

__init__():
    if x_missing:
        raíse RuntimeError("nice error message.....")

That could be used in REPL as well, there are a number of dependencies there.

janiversen · 2023-10-17T17:51:06Z

Forgot to write using RuntimeError secures the exception is not caught in a wildcard except. I think runtime error is the only exception not catchable.

Consistent ImportError messages

a5341c1

alexrudd2 force-pushed the import-errors branch from f51e4a0 to a5341c1 Compare October 17, 2023 05:05

janiversen requested changes Oct 17, 2023

View reviewed changes

janiversen mentioned this pull request Oct 17, 2023

eliminates implicit Optional #1817

Closed

alexrudd2 marked this pull request as draft October 17, 2023 16:30

alexrudd2 mentioned this pull request Oct 17, 2023

Consistent messages if imports fail #1831

Merged

alexrudd2 closed this Oct 17, 2023

alexrudd2 deleted the import-errors branch October 17, 2023 17:54

github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2023

Consistent ImportError messages #1827

Consistent ImportError messages #1827

Uh oh!

Conversation

alexrudd2 commented Oct 17, 2023

Uh oh!

janiversen Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

janiversen Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

alexrudd2 Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

janiversen Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

janiversen Oct 17, 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!

alexrudd2 Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

alexrudd2 commented Oct 17, 2023

Uh oh!

janiversen commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexrudd2 commented Oct 17, 2023

Uh oh!

janiversen commented Oct 17, 2023

Uh oh!

janiversen commented Oct 17, 2023

Uh oh!

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

Uh oh!

Uh oh!

janiversen commented Oct 17, 2023

Uh oh!

alexrudd2 commented Oct 17, 2023

Uh oh!

janiversen commented Oct 17, 2023

Uh oh!

alexrudd2 commented Oct 17, 2023

Uh oh!

janiversen commented Oct 17, 2023

Uh oh!

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

Uh oh!

Uh oh!

alexrudd2 commented Oct 17, 2023

Uh oh!

janiversen commented Oct 17, 2023

Uh oh!

janiversen commented Oct 17, 2023

Uh oh!

Uh oh!

janiversen Oct 17, 2023 •

edited

Loading

janiversen commented Oct 17, 2023 •

edited

Loading

alexrudd2 commented Oct 17, 2023 •

edited

Loading

alexrudd2 commented Oct 17, 2023 •

edited

Loading