Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bad calibration result fails in web serialization #235

Open
amitschang opened this issue Dec 18, 2024 · 7 comments · May be fixed by #237
Open

Bad calibration result fails in web serialization #235

amitschang opened this issue Dec 18, 2024 · 7 comments · May be fixed by #237
Assignees
@amitschang
Labels
app bug Something isn't working p0-priority-high

Comments

@amitschang
Copy link
Member

amitschang commented Dec 18, 2024 •
edited
Loading

Using the default calibration for OD90 seems to have parameters incompatible with our sensors, in that for some vials nan is produced, and this then results in crashing the application (not just exception isolated to endpoint) [UI being unusable for device], with error like (snipped for brevity):

Dec 18 20:40:08 raspberrypi python[857893]: ERROR:    Exception in ASGI application                                                                                          
Dec 18 20:40:08 raspberrypi python[857893]: Traceback (most recent call last):                                                                                               
Dec 18 20:40:08 raspberrypi python[857893]:   File "/home/pi/evolver-venv/lib/python3.11/site-packages/uvicorn/protocols/http/httptools_impl.py", line 399, in run_asgi      
<<snipped>>
Dec 18 20:40:08 raspberrypi python[857893]:   File "/home/pi/evolver-venv/lib/python3.11/site-packages/starlette/responses.py", line 187, in render
Dec 18 20:40:08 raspberrypi python[857893]:     return json.dumps(
Dec 18 20:40:08 raspberrypi python[857893]:            ^^^^^^^^^^^
Dec 18 20:40:08 raspberrypi python[857893]:   File "/usr/lib/python3.11/json/__init__.py", line 238, in dumps
Dec 18 20:40:08 raspberrypi python[857893]:     **kw).encode(obj)
Dec 18 20:40:08 raspberrypi python[857893]:           ^^^^^^^^^^^
Dec 18 20:40:08 raspberrypi python[857893]:   File "/usr/lib/python3.11/json/encoder.py", line 200, in encode
Dec 18 20:40:08 raspberrypi python[857893]:     chunks = self.iterencode(o, _one_shot=True)
Dec 18 20:40:08 raspberrypi python[857893]:              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Dec 18 20:40:08 raspberrypi python[857893]:   File "/usr/lib/python3.11/json/encoder.py", line 258, in iterencode
Dec 18 20:40:08 raspberrypi python[857893]:     return _iterencode(o, 0)
Dec 18 20:40:08 raspberrypi python[857893]:            ^^^^^^^^^^^^^^^^^
Dec 18 20:40:08 raspberrypi python[857893]: ValueError: Out of range float values are not JSON compliant

Using the default calibration file and values like those read on the device, I can confirm that the fit function produces a NaN. While this is probably the correct thing to do in such a case, we should ensure that the app can handle or gracefully error with such values.

Oddly, just doing this on python prompt produces non-error looking results:

>>> json.dumps(np.nan)
'NaN'

and

>>> class X(pydantic.BaseModel):
...   x: float
>>> X(x=np.nan).model_dump_json()
'{"x":null}'

So there may be something fastapi specific going on here...
@amitschang amitschang added bug Something isn't working app p0-priority-high labels Dec 18, 2024
@jamienoss
Copy link
Member

It shouldn't "crash" the application, does it?

@amitschang
Copy link
Member Author

True, I'm not sure why I wrote that. The process is running but the UI is rendered broken

image

@amitschang amitschang changed the title Bad calibration result crashes application Bad calibration result fails in web serialization Dec 19, 2024
@jamienoss
Copy link
Member

Ok, that's ok, I would state the same thing for this. The application should still be usable. We prob just need to add/activate the error handlers and/or better invocation of uvicorn so that it auto restarts.

@amitschang
Copy link
Member Author

Shouldn't we fix so that the NaNs end up serialized as nulls? Or otherwise in a way where the state endpoint can still be called?

@jamienoss
Copy link
Member

Sure, but isn't this ticket 2 in 1? One issue being the NaN and the other the lack of fault tolerance on the app, right?

@amitschang
Copy link
Member Author

Found this: fastapi/fastapi#459, looks like it is similar issue. I could reproduce in a test case and see it comes from code in JSONEncoder:

            if not allow_nan:
                raise ValueError(
                    "Out of range float values are not JSON compliant: " +
                    repr(o))

default must be to allow_nan=False (doesn't look like it is specifcally stated in fastapi).

And no, I think it is just the one issue. Sorry for the confusion, UI seems to be more sensitive than it needs (for instance the "online" indicator should only hit /healthz, but maybe does more)

@amitschang amitschang self-assigned this Dec 19, 2024
@amitschang amitschang linked a pull request Dec 19, 2024 that will close this issue
Open
@jamienoss
Copy link
Member

And no, I think it is just the one issue. Sorry for the confusion, UI seems to be more sensitive than it needs (for instance the "online" indicator should only hit /healthz, but maybe does more)

Ah ok, cool.

@imaitland can yo take a look and see what the issue there might be, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@amitschang amitschang

Labels
app bug Something isn't working p0-priority-high
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

switch json response handler to enable NaN in serialization ssec-jhu/evolver-ng
2 participants
@jamienoss @amitschang