epics-base / p4p

Python bindings for the PVAccess network client and server.
BSD 3-Clause "New" or "Revised" License
27 stars 38 forks source link

Put builder failure reported but not raised #115

Closed coretl closed 10 months ago

coretl commented 1 year ago

When the Put builder fails, it is reported to the console, but the call to Context.put does not raise an exception. This was somewhat surprising!

For example, when running a database:

# demo.db
record(waveform, "demo:pv:name") {
    field(NELM, "256")
    field(FTVL, "LONG")
    field(INP, [1,2,3])
}

With IOC python -m epicscorelibs.ioc -d demo.db, then running client script:

from p4p.client.thread import Context
import numpy as np

pv = 'demo:pv:name'

with Context("pva", unwrap=False) as c:
    print(f"Initial: {c.get(pv).value}")
    c.put(pv, [4, 5, 6])
    print(f"List works: {c.get(pv).value}")
    c.put(pv, np.array([7, 8, 9]))
    print(f"Np.int64 array doesn't: {c.get(pv).value}")

I get the following output, showing the put fails but does not raise:

$ python ./p4p_client.py 
Initial: [1 2 3]
List works: [4 5 6]
Exception in Put builder
Traceback (most recent call last):
  File "/venv/lib/python3.10/site-packages/p4p/client/raw.py", line 80, in builder
    nt.assign(V, value)
  File "/venv/lib/python3.10/site-packages/p4p/nt/__init__.py", line 84, in assign
    self._assign(V, value)
  File "/venv/lib/python3.10/site-packages/p4p/nt/__init__.py", line 100, in _default_assign
    V.value = value # assume NTScalar-like
  File "src/p4p/_p4p.pyx", line 223, in p4p._p4p._Value.__setattr__
TypeError: Cannot cast array data from dtype('int64') to dtype('int32') according to the rule 'safe'
Unhandled Exception src/pvxs_client.cpp:67
Traceback (most recent call last):
  File "/venv/lib/python3.10/site-packages/p4p/client/raw.py", line 80, in builder
    nt.assign(V, value)
  File "/venv/lib/python3.10/site-packages/p4p/nt/__init__.py", line 84, in assign
    self._assign(V, value)
  File "/venv/lib/python3.10/site-packages/p4p/nt/__init__.py", line 100, in _default_assign
    V.value = value # assume NTScalar-like
  File "src/p4p/_p4p.pyx", line 223, in p4p._p4p._Value.__setattr__
TypeError: Cannot cast array data from dtype('int64') to dtype('int32') according to the rule 'safe'
Np.int64 array doesn't: [4 5 6]

I get the same result if I use p4p.client.asyncio

Running on Linux with python 3.10.6, p4p 4.1.9, pvxslibs 1.2.2, epicscorelibs 7.0.7.99.0.2

mdavidsaver commented 1 year ago

I see two issues here:

coretl commented 1 year ago

Excellent, thanks for doing this, having the put builder exceptions propagate upwards will make it a lot easier to spot the problem in unit tests where pytest has swallowed stdout...

To make it clear, I wasn't regarding the existing casting behavior as a bug, just the lack of exception. Personally, I don't know whether I prefer the safe or unsafe casting rules. On the one hand np.ndarray([1,2,3]) looks like is should be allowed for an int32 field, but on the other hand np.ndarray([1.5, 2.5, 3.5]) probably shouldn't. I guess the new behavior will allow both and truncate the floats. Maybe I would argue that the safe rules are less surprising? Although I could be convinced otherwise...

mdavidsaver commented 1 year ago

To make it clear, I wasn't regarding the existing casting behavior as a bug ...

Understood. I however do look upon stronger coupling of data types between server and clients (I assume plural) as a potential maintenance problem. cf. the problems caused by older versions of normativeTypesCPP. So to my mind the default should be permissive, as is already the case for scalar values.

A client which wants to ensure fidelity can provide a put builder callback and inspect the types. Of course this is not so convenient. I could see adding a way to select stricter casting, although designing such an interface runs into some of the same problems as eg. timestamps wrt. the NT wrappers. (although maybe we don't bother with the wrappers initially)

mdavidsaver commented 10 months ago

I think this issue has been addressed.