clarified source of an error message#1086
Open
bruceravel wants to merge 1 commit intobluesky:mainfrom
Open
Conversation
in the put method to the EpicsSignal class, there is a check to be sure that the signal can be written to, i.e. that self.write_access is true. that error message results in an unhelpful traceback in that it provides no information identifying the PV triggering the message. in the case of a deeply nested signal, this error message is unilluminating. this commit adds self.name to the error message.
prjemian
reviewed
Jan 3, 2023
|
|
||
| if not self.write_access: | ||
| raise ReadOnlyError("No write access to underlying EPICS PV") | ||
| raise ReadOnlyError(f"No write access to underlying EPICS PV : {self.name}") |
Contributor
There was a problem hiding this comment.
Since this is an EPICS signal, can you also add the write_pvname to the message? It's one of the diagnostics our instrument teams want to see in such a message.
Contributor
There was a problem hiding this comment.
Also, the message would be helped by putting the write PV name first as stated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
in the put method to the EpicsSignal class, there is a check to be sure that the signal can be written to, i.e. that self.write_access is true. that error message results in an unhelpful traceback in that it provides no information identifying the PV triggering the message. in the case of a deeply nested signal, this error message is unilluminating. this commit adds self.name to the error message.
This is, in my opinion, an example of a common anti-pattern in ophyd. There are many error messages that do a poor job identifying the prima causa of the error, simply resulting in a lengthy traceback that is hard to parse for someone not well versed in ophyd's internals.
In my case, I was, with a lot of poking and prodding, able to identify the source of the error. Had the error message included the name (as in
self.name) of the ophyd object, it would have been a much shorter path to identifying the problem.My solution was to simply append
self.nameto the error message. Not a perfect solution, but it would at least point the human doing the debugging in the right direction.I have a hard time believing this is simply a problem for someone like me, who is writing beamline code without also having a deep understanding of Ophyd. I cannot believe that a context-free error message could be helpful even to Ophyd's main developers.
Were I king of the world (or at least of DSSI), I would call for a full audit of ophyd to find all examples of context-free messages like this one. I would then call for agreement on a convention for making better, more helpful, better contextualized error messages.
All right! Rant is finished. Thanks for humoring me 😁