FIX: Consistent treatment of timestamps in Gantt chart visualization #2769
Conversation
There was a problem hiding this comment.
Hi @dPys. Sorry for taking so long to look at this. If you're interested in getting this in for the next release, we'll need to get the tests working. I had a quick read through, and have some comments. Have you tested this locally?
Also, you'll almost certainly want to merge master at this point.
nipype/utils/draw_gantt_chart.py
Outdated
| @@ -81,6 +83,9 @@ def create_event_dict(start_time, nodes_list): | |||
| events[start_delta] = start_node | |||
| events[finish_delta] = finish_node | |||
|
|
|||
| del node_start | |||
| del node_finish | |||
There was a problem hiding this comment.
It shouldn't be necessary to delete these objects.
nipype/utils/draw_gantt_chart.py
Outdated
| all_res += float(event[resource]) | ||
| try: | ||
| all_res += float(event[resource]) | ||
| except: |
There was a problem hiding this comment.
What's the exception-producing case?
nipype/utils/draw_gantt_chart.py
Outdated
| 'node_start': node_start.strftime("%Y-%m-%d %H:%M:%S"), | ||
| 'node_finish': node_finish.strftime("%Y-%m-%d %H:%M:%S") | ||
| 'node_start': node_start, | ||
| 'node_finish': node_finish |
There was a problem hiding this comment.
This seems to be replacing a string with a datetime object. Is that intended? It will now be formatted with str(node_start) and str(node_finish) in the format string below:
new_node = "<div class='node' style='left:%(left)spx;top:%(offset)spx;"\
...
"start:%(node_start)s\nend:%(node_finish)s'></div>" %
node_dictI'm not sure that's desirable. strftime is insensitive to changes in datetime.__str__ across versions.
nipype/utils/draw_gantt_chart.py
Outdated
| @@ -304,6 +316,9 @@ def draw_nodes(start, nodes_list, cores, minute_scale, space_between_minutes, | |||
| # Append to output result | |||
| result += new_node | |||
|
|
|||
| del node_start | |||
| del node_finish | |||
There was a problem hiding this comment.
Again, no real need to delete these that I can see.
nipype/utils/draw_gantt_chart.py
Outdated
| 'start': ts_start.strftime('%Y-%m-%d %H:%M:%S'), | ||
| 'finish': ts_end.strftime('%Y-%m-%d %H:%M:%S') | ||
| 'start': ts_start, | ||
| 'finish': ts_end |
nipype/utils/profiler.py
Outdated
| @@ -138,14 +137,20 @@ def log_nodes_cb(node, status): | |||
| import logging | |||
| import json | |||
|
|
|||
| # Address nested case | |||
| if isinstance(node.result.runtime, (list,)): | |||
| resrunlist = node.result.runtime[0] | |||
There was a problem hiding this comment.
resrunlist is a single runtime, not a list. Let's just go with result_runtime as variable name.
nipype/utils/profiler.py
Outdated
| # Address nested case | ||
| if isinstance(node.result.runtime, (list,)): | ||
| resrunlist = node.result.runtime[0] | ||
| except: |
effigies
changed the title
|
Hi, just a reminder that the 1.1.8 release is targeted for January 28. Please let us know if you'd like to try to finish this up for that release. |
Timestamps in run_stats.log are saved as strings, but previously these functions directly treated the timestamps as datetime objects. This has now been corrected across all functions.
Summary
Fixes # .
List of changes proposed in this PR (pull-request)
Acknowledgment