-
Notifications
You must be signed in to change notification settings - Fork 1
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
Log replay #215
Log replay #215
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
invertedai/logs/common.py
Outdated
json.dump(output_dict, outfile) | ||
|
||
@classmethod | ||
def write_json_from_scenario_log( |
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 seems to be doing the same thing as write_scenario_log_to_json
? And it doesn't make any sense to pass a cls to an instance method. I am not sure if this will work without passing a concrete object to it
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.
Tested this and it indeed works. I can see it being useful if a user doesn't want to create a LogWriter object just to write the log to disk.
invertedai/logs/common.py
Outdated
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 module is called common but you are importing stuff from one level above. This might easily cause circular imports in the future and break the package level.
Also having docstrings for each method would be very helpful
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.
What name would you suggest? Perhaps logger.py, logging.py, logs.py, log_utils.py, anything else?
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 can add the docstrings for sure.
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 don't really have much further to add, i will let @rf-ivtdai to take another look
Added tools to be able to conveniently read and write logs in the python SDK.