Scottx611x/refactor attach outputs dataset #2591
Conversation
…tach_derived_nodes_to_dataset`
Codecov Report
@@ Coverage Diff @@
## develop #2591 +/- ##
===========================================
- Coverage 55.58% 53.03% -2.56%
===========================================
Files 408 406 -2
Lines 29474 26437 -3037
Branches 1240 1240
===========================================
- Hits 16384 14021 -2363
+ Misses 13090 12416 -674
Continue to review full report at Codecov.
|
refinery/core/models.py
Outdated
| @@ -1161,6 +1161,12 @@ class Meta: | |||
| ) | |||
| ordering = ['-time_end', '-time_start'] | |||
|
|
|||
| @property | |||
There was a problem hiding this comment.
What's the benefit of making this a property?
There was a problem hiding this comment.
No benefit, changing it now
refinery/core/models.py
Outdated
| ast.literal_eval(self.workflow_copy) | ||
| def attach_derived_nodes_to_dataset(self): | ||
| graph_with_data_transformation_nodes = ( | ||
| self._create_data_transformation_nodes(self.workflow_graph) |
There was a problem hiding this comment.
We just talked about this... probably more clear just to call?
refinery/core/models.py
Outdated
| node_name = "{}_{}".format( | ||
| data_transformation_node['tool_id'], | ||
| data_transformation_node['name'] | ||
| ) |
There was a problem hiding this comment.
maybe:
"{tool_id}_{name}".format(**data_transformation_node)and then it's almost short enough to just put in the create below.
refinery/core/models.py
Outdated
|
|
||
| if graph[edge[0]][edge[1]]['output_id'] == input_id: | ||
| input_node_id = edge[1] | ||
| data_transformation_node = ( |
There was a problem hiding this comment.
I would tend to just back-slash in a case like this... maybe to avoid the closing paren, or the risk of a stray comma turning it into a tuple, but this could be the wrong call.
refinery/core/models.py
Outdated
| @@ -1161,6 +1161,11 @@ class Meta: | |||
| ) | |||
| ordering = ['-time_end', '-time_start'] | |||
|
|
|||
| def get_workflow_graph(self): | |||
refinery/core/models.py
Outdated
| output_connection_to_analysis_result_mapping = ( | ||
| self._get_output_connection_to_analysis_result_mapping() | ||
| ) | ||
| output_mappings = output_connection_to_analysis_result_mapping |
There was a problem hiding this comment.
just use this name in the first place?
refinery/core/models.py
Outdated
| "Results for '%s' and %s.%s: %s", | ||
| self.uuid, | ||
| output_connection, | ||
| output_connection.filetype, |
There was a problem hiding this comment.
Below, we debug with just output_connection... maybe that's enough?
There was a problem hiding this comment.
I don't think the repr() of output_connection has the filetype information
refinery/core/models.py
Outdated
| a. attach output node to source data transformation node | ||
| b. attach output node to target data transformation node | ||
| (if exists)""" | ||
| if len(graph.edges([output_connection.step])) > 0: |
There was a problem hiding this comment.
but if it were zero, then the loop below would just exit quickly?
refinery/core/models.py
Outdated
|
|
||
| if graph[edge[0]][edge[1]]['output_id'] == output_id: | ||
| input_node_id = edge[0] | ||
| output_node_id = edge[1] |
There was a problem hiding this comment.
a lot like line 1681? maybe define the variables first?
| # any children | ||
| if (not output_connection.is_refinery_file and | ||
| derived_data_file_node.children.count() == 0): | ||
| output_connection.node.delete() |
There was a problem hiding this comment.
maybe:
if derived_data_file_node.children.count() == 0:
if output_connection.is_refinery_file:
...
else:
...There was a problem hiding this comment.
I thought we could make the split here too, but looking at theses two ifs one is worried about derived_data_file_node.parents and the other derived_data_file_node.children
I was in this area of the code today and figured it was high time for some refactoring
Fixes: #2415