-
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
Add a script to make review of pipeline logs easier #6
base: master
Are you sure you want to change the base?
Conversation
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.
As in comments, I think it would be great if this could use generators everywhere with no intermediate lists to make streaming/filtering the log file(s) as efficient as possible. I think all of the work to filter/format lines can be done on a line-by-line basis?
aodncore/bin/logview.py
Outdated
""" | ||
# read all log lines | ||
with open(logfile) as log: | ||
lines = log.readlines() |
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.
If you get rid of this readlines(), and just use "for line in log:" below it will have the same result but use the built in iterator and be much more memory efficient and probably faster (especially for large files).
aodncore/bin/logview.py
Outdated
|
||
m = INPUT_REGEX.match(line) | ||
if m is not None: | ||
log_data.append(m.groupdict()) |
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.
Same here as above.. I think making this run off a generator and yield matching lines rather than creating intermediate lists would be more efficient.
aodncore/bin/logview.py
Outdated
""" | ||
output = [] | ||
for data in log_data: | ||
output.append(fmt.format(**data)) |
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.
Same here, format while iterating to avoid intermediate list.
Thanks @lwgordonimos I think you mentioned that earlier, and I've been working on changes along those lines, just haven't pushed them up yet. I'll get back to this soon. |
@mhidas I'm doing a sweep of outstanding PRs and this one looks relatively old. Can this be closed or left as open? |
I'd like to actually get this script to a somewhat useable state and deploy it. Just haven't had time to work on it. @lbesnard @bpasquer @ggalibert Would you find such a script useful? It would be similar to the |
i dunno exactly how this would work, but having an autocomplete on the list of handlers would be nice |
Autocompletion will be simple, probably by implemented a simple command line utility in aodncore. |
Agreed, this would be great. With a behaviour similar to the |
DO NOT MERGE WIP / Proof of concept
Trying to address some of the requirements described in https://github.com/aodn/zzz-aodn-pipeline-poc/issues/143