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.

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

Improve error handling/reporting with subprocess #503

Open
joeshannon opened this issue Jun 12, 2024 · 7 comments
Open

Improve error handling/reporting with subprocess #503

joeshannon opened this issue Jun 12, 2024 · 7 comments
Labels
bug Something isn't working good first issue Good for newcomers
Milestone

Comments

@joeshannon
Copy link
Contributor

The worker has been moved to the subprocess but when there is an error it can be hard to see as it is tangled up in the subprocess stack as well.

We should see if this can be improved.

@joeshannon
Copy link
Contributor Author

This should cover the issue raised in DiamondLightSource/i22-bluesky#35

@DominicOram
Copy link
Contributor

We found an issue on i04 last week where we got the name wrong on an inject statement in the plan parameter definition. This caused a message of the form:

multiprocessing.pool.MaybeEncodingError: Error sending result: '<multiprocessing.pool.ExceptionWithTraceback object at 0x7f519d0d2350>'. Reason: 'PicklingError("Can't pickle <class 'pydantic.main.thaw_and_stream_to_murko'>: attribute lookup thaw_and_stream_to_murko on pydantic.main failed")'

Where thaw_and_stream_to_murko was the plan we were trying to run. It wasn't clear that this was due to the bad name in inject at all.

@joeshannon
Copy link
Contributor Author

Now that #584 is merged the error message might be different (better?) now. This removed some pickling but I'm not sure if it removed all pickling.
Although I'm not sure as I'm not working on blueapi anymore but possibly @callumforrester or @DiamondJoseph might know.

@callumforrester
Copy link
Contributor

Blueapi had not been released for some time due to CI pipeline issues, I have made a new alpha release: https://github.com/DiamondLightSource/blueapi/releases/tag/0.4.5-a3

It may be worth testing with that when you get the chance @DominicOram

@callumforrester
Copy link
Contributor

Another instance of this problem:

error_message=str(e),

It looks like str(stomp.ConnectFailedException()) is an empty string, so that's not helpful...

Should maybe change it to str(type(e)) + ": " + str(e)?

@DominicOram
Copy link
Contributor

This happened again on I04 today, we were accidentally passing in a None to a parameter on a plan which expected an int. The error returned was | multiprocessing.pool.MaybeEncodingError: Error sending result: '<multiprocessing.pool.ExceptionWithTraceback object at 0x7fe5ea561d50>'. Reason: 'PicklingError("Can't pickle <class 'pydantic.main.thaw'>: attribute lookup thaw on pydantic.main failed")'. This was still the older blueapi version though

@DiamondJoseph
Copy link
Contributor

DiamondJoseph commented Sep 9, 2024

Just chucked some nonsense at a server on main:
E: This comment misses the mark a fair bit: all of these errors are being caught by the Pydantic validation of models as they are deserialised at the REST API. The issue is when a malformed argument bypasses the validation (e.g. with current inject handling, or any default argument with #type: ignore: I would try writing plans with bad arguments and not passing any overrides instead.

1: count w/ {"detectors": [null]}

{
  "detail": "\n        Input validation failed: detectors: Input should be a valid string,\n        suppplied params {'detectors': [None]},\n        do not match the expected params: {'additionalProperties': False, 'properties': {'detectors': {'items': {'type': 'bluesky.protocols.Readable'}, 'title': 'Detectors', 'type': 'array', 'uniqueItems': True}, 'num': {'title': 'Num', 'type': 'integer'}, 'delay': {'anyOf': [{'type': 'number'}, {'items': {'type': 'number'}, 'type': 'array'}, {'type': 'null'}], 'title': 'Delay'}, 'metadata': {'anyOf': [{'type': 'object'}, {'type': 'null'}], 'title': 'Metadata'}}, 'required': ['detectors'], 'title': 'count', 'type': 'object'}\n        "
}

2: count w/ {"detectors": [null], "num": "foo"}

{
  "detail": "\n        Input validation failed: detectors: Input should be a valid string; num: Input should be a valid integer, unable to parse string as an integer,\n        suppplied params {'detectors': [None], 'num': 'foo'},\n        do not match the expected params: {'additionalProperties': False, 'properties': {'detectors': {'items': {'type': 'bluesky.protocols.Readable'}, 'title': 'Detectors', 'type': 'array', 'uniqueItems': True}, 'num': {'title': 'Num', 'type': 'integer'}, 'delay': {'anyOf': [{'type': 'number'}, {'items': {'type': 'number'}, 'type': 'array'}, {'type': 'null'}], 'title': 'Delay'}, 'metadata': {'anyOf': [{'type': 'object'}, {'type': 'null'}], 'title': 'Metadata'}}, 'required': ['detectors'], 'title': 'count', 'type': 'object'}\n        "
}

3: 'count w/ {"detectors": null}`

{
  "detail": "\n        Input validation failed: detectors: Input should be a valid set,\n        suppplied params {'detectors': None},\n        do not match the expected params: {'additionalProperties': False, 'properties': {'detectors': {'items': {'type': 'bluesky.protocols.Readable'}, 'title': 'Detectors', 'type': 'array', 'uniqueItems': True}, 'num': {'title': 'Num', 'type': 'integer'}, 'delay': {'anyOf': [{'type': 'number'}, {'items': {'type': 'number'}, 'type': 'array'}, {'type': 'null'}], 'title': 'Delay'}, 'metadata': {'anyOf': [{'type': 'object'}, {'type': 'null'}], 'title': 'Metadata'}}, 'required': ['detectors'], 'title': 'count', 'type': 'object'}\n        "
}

4: count w/ {"detectors": ["x"], "num": null}

{
  "detail": "\n        Input validation failed: num: Input should be a valid integer,\n        suppplied params {'detectors': ['x'], 'num': None},\n        do not match the expected params: {'additionalProperties': False, 'properties': {'detectors': {'items': {'type': 'bluesky.protocols.Readable'}, 'title': 'Detectors', 'type': 'array', 'uniqueItems': True}, 'num': {'title': 'Num', 'type': 'integer'}, 'delay': {'anyOf': [{'type': 'number'}, {'items': {'type': 'number'}, 'type': 'array'}, {'type': 'null'}], 'title': 'Delay'}, 'metadata': {'anyOf': [{'type': 'object'}, {'type': 'null'}], 'title': 'Metadata'}}, 'required': ['detectors'], 'title': 'count', 'type': 'object'}\n        "
}

5: count w/ {"detectors": ["non_existent"]}

{
  "detail": "\n        Input validation failed: detectors: Value error, Device non_existent is not of type <class 'bluesky.protocols.Readable'>,\n        suppplied params {'detectors': ['non_existent']},\n        do not match the expected params: {'additionalProperties': False, 'properties': {'detectors': {'items': {'type': 'bluesky.protocols.Readable'}, 'title': 'Detectors', 'type': 'array', 'uniqueItems': True}, 'num': {'title': 'Num', 'type': 'integer'}, 'delay': {'anyOf': [{'type': 'number'}, {'items': {'type': 'number'}, 'type': 'array'}, {'type': 'null'}], 'title': 'Delay'}, 'metadata': {'anyOf': [{'type': 'object'}, {'type': 'null'}], 'title': 'Metadata'}}, 'required': ['detectors'], 'title': 'count', 'type': 'object'}\n        "
}

I think 1,2,3,4 are all fine, 5 could maybe be better at noting that the device doesn't exist and isn't just a non-Readable, especially if it could be an injected value that is mispelt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants