Handle poorly configured streams #1510
Conversation
dangunter
requested review from
andrewlee94,
dallan-keylogic and
lbianchi-lbl
as code owners
|
Before looking at the code changes: do you have an idea of how much work it would be to add a test exercising this error handling, i.e. a model with missing and/or invalid data that without this PR would result in an error when creating the stream table? |
| disp_dict = sb.define_state_vars() | ||
| else: | ||
| disp_dict = sb.define_display_vars() | ||
| except AttributeError: # if define_(display,state)_vars doesn't exist(!) |
There was a problem hiding this comment.
Have we actually seen cases of this? In order for define_state_vars not to exist then someone must have done something wrong and maybe we should be catching this earlier.
There was a problem hiding this comment.
However if you can suggest where "earlier" might be, I can try to catch it there
There was a problem hiding this comment.
So, I think this might need a bigger design discussion. The issue here is the the stream table methods were written only to consider things that were in "material streams", i.e. those related to state blocks in which case the define_state_vars method must exist.
What Mayo was doing was trying to add additional, non-state, data to the stream table which is not supported at the moment. I think a better solution to this might be to design an interface for adding this additional information rather than trying to shoehorn it into the existing interface. There were a few other catches to what Mayo was trying to do as well due to how state data is indexed versus things at the unit level that need to be resolved as well.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1510 +/- ##
==========================================
- Coverage 76.99% 76.98% -0.02%
==========================================
Files 382 382
Lines 61992 62002 +10
Branches 10146 10146
==========================================
Hits 47730 47730
- Misses 11854 11865 +11
+ Partials 2408 2407 -1 ☔ View full report in Codecov by Sentry. |
| disp_dict = sb.define_state_vars() | ||
| else: | ||
| disp_dict = sb.define_display_vars() | ||
| except AttributeError: # if define_(display,state)_vars doesn't exist(!) |
There was a problem hiding this comment.
Perhaps refactor to
try:
if true_state:
disp_func = sb.define_state_vars
else:
disp_func = sb.define_display_vars
except AttributeError:
disp_func = None
....
if disp_func is not None:
disp_dict = disp_func()
else:
disp_dict = {}
That way we catch only the attribute error for the function not existing, but not attribute errors internal to the function.
There was a problem hiding this comment.
Actually it looks like you can just use getattr and provide a default to avoid the try-except block. I'm not sure whether None would work as a default, though.
There was a problem hiding this comment.
disp_func = lambda: {} should work for that.
dangunter
requested review from
andrewlee94,
OOAmusat and
dallan-keylogic
and removed request for
andrewlee94,
lbianchi-lbl and
dallan-keylogic
|
@dangunter is still discussing whether this should be handled in the core (i.e. in this repository), or on the UI side. |
Fixes #1509
Changes proposed in this PR:
display_{state,display}_vars, set dict to empty and issue a warningLegal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: