-
Notifications
You must be signed in to change notification settings - Fork 9
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
Dashboard improvements #109
Conversation
… printing in communication protocol
error_text = 'The memoization KVS is not configured.\n'\ | ||
'Please add a `memoization` kvs item to the `kvs` '\ | ||
'key in `creds.yaml`.\n'\ | ||
'```\nmemoization:\n type: redis_ephemeral\n expiry: 60\n```' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also include the stub option.
This is important, since we want to make sure the system works without redis as well.
|
||
def to_dict(self): | ||
# TODO create serialize/deserialize methods for traceback | ||
tb = self.traceback | ||
if hasattr(self, '__traceback__'): | ||
tb = self.__traceback__ if tb is None else tb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random thoughts (not a demand for a change, but thinking points):
We might consider checking settings about whether to do this. I'm not sure where Python is right now (things might have improved), but performance-wise, generating tracebacks used to be super-expensive in the olden days.
It might be important to do this for log files, though.
At the time, a lot of systems would do this stochastically for log files (e.g. log details of 1% of errors at random for debugging).
''' | ||
errors = [] | ||
|
||
def recurse(item): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better function name. Also, docstring.
@@ -198,6 +198,8 @@ def clean_json(json_object): | |||
return str(json_object) | |||
if str(type(json_object)) == "<class 'learning_observer.runtime.Runtime'>": | |||
return str(json_object) | |||
if str(type(json_object)) == "<class 'dict_keys'>": | |||
return str(json_object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a list(json_object)
, I think.
update_error_storage: function (message) { | ||
const errors = {}; | ||
for (const key in message.wo) { | ||
if (Object.prototype.hasOwnProperty.call(message.wo[key], 'error')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See if ?.error !== undefined
or simply if(message.wo[key]?.error)
Or give a pointer to why this is preferred.
return ['', false, '']; | ||
} | ||
const text = 'Oops! Something went wrong ' + | ||
"on our end. We've noted the " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Pick a style of quotation marks and stick with it.
*/ | ||
update_alert_with_error: function (error) { | ||
if (!error) { | ||
return ['', false, '']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd comment what this means.
/* Text to display: / '', / Alert displayed / false, / JSON of error on the alert (only in debug): */, ''
if (!data | data.length === 0 | Object.prototype.hasOwnProperty.call(data, 'error')) { | ||
return ['Select a student', '', [], [], 'individual-student-loaded'] | ||
return ['Select a student', '', [], [{}, [0], 0], 'individual-student-loaded'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document data types in lists generally, at least for more mature code.
} | ||
const data = JSON.parse(msg.data).docs_with_nlp.nlp_combined; | ||
|
||
if (!Object.prototype.hasOwnProperty.call(data, 'error')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Abstract error out. Ideally, we'd have an <Error>
component to include in our layouts which does all the right stuff.
This can go on backlog, but it should be documented.
No description provided.