logging toggle with command line #79
Conversation
toggle the logging with --debug flags
There was a problem hiding this comment.
I've reviewed the pull and I have one general comment and a couple specific requests. Ultimately I think we may need to go back to the drawing board on this one.
The general comment is that this is a workable patch for getting logging working in the ModularAgent but it doesn't solve the general problem for the rest of the library that use logging in different ways. Part of the issue is that we don't have a consistent style for using the logging module and we may want to design that first before introducing a solution like this.
Specific Requests:
- Remove the apprentice/agents/.DS_Store file from the commit and maybe at .DS_Store to the .gitignore file
- Fix the find-replaced uses of the logging library that throw errors because of not concatenating arguments like print.
|
|
||
| debugging_state = os.environ.get('DEBUGGING_STATE') | ||
| debugging_agent = os.environ.get('DEBUGGING_AGENT') |
There was a problem hiding this comment.
While this is a workable patch to the problem of adding command line controls for debugging, I'd ideally like to see a more general solution. These loggers already have names 'al-performance' and 'al-agent' so rather than creating new tags that have to be maintained in some space can we just use the existing tags in someway? Additionally, different components of the broader library use loggers differently, (usually using the common convention of the a logger named after the class path). Ideally a solution for the whole library would be able to support that use case as well.
| @@ -634,6 +664,7 @@ def train(self, state:Dict, sai:Sai=None, reward:float=None, | |||
| rhs_by_how = self.rhs_by_how.get(skill_label, {}) | |||
| for exp in explanations: | |||
| # print("FOUND EX:", str(exp)) | |||
| agent_logger.error("FOUND EX:", str(exp)) | |||
There was a problem hiding this comment.
The logging library's various message functions don't automatically concatenate arguments in the same way that print does so it can't be used as a drop-in replacement like this. See: https://docs.python.org/3/library/logging.html#logging.Logger.debug. When I run this code on my end I mostly just see a bunch of exception stack traces.
| @@ -665,6 +696,7 @@ def check(self, state, selection, action, inputs): | |||
| responses = resp['responses'] | |||
| for resp in responses: | |||
| # print(resp['selection'],resp['action'],resp['inputs'], _inputs_equal(resp['inputs'],inputs)) | |||
| agent_logger.error(resp['selection'],resp['action'],resp['inputs'], _inputs_equal(resp['inputs'],inputs)) | |||
There was a problem hiding this comment.
I would be careful of logger constructions like this (i.e., that make function calls inside the log call) because those calls will be evaluated whether or not the logger is active. It would be good to gate these behind a check that the appropriate level is active.
toggle the logging with --debug flags