-
Notifications
You must be signed in to change notification settings - Fork 80
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
fix add_default_workflow #3446
base: dev
Are you sure you want to change the base?
fix add_default_workflow #3446
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -808,14 +808,24 @@ def _get_node_info(workflow, node): | |
|
||
parent_cmd_name = None | ||
parent_merging_scheme = None | ||
phms = None | ||
if pcmd is not None: | ||
parent_cmd_name = pcmd.name | ||
parent_merging_scheme = pcmd.merging_scheme | ||
if not parent_merging_scheme['ignore_parent_command']: | ||
phms = _get_node_info(workflow, parent) | ||
|
||
return qdb.util.human_merging_scheme( | ||
hms = qdb.util.human_merging_scheme( | ||
ccmd.name, ccmd.merging_scheme, parent_cmd_name, | ||
parent_merging_scheme, cparams, [], pparams) | ||
|
||
# if the parent should not ignore its parent command, then we need | ||
# to merge the previous result with the new one | ||
if phms is not None: | ||
hms = qdb.util.merge_overlapping_strings(hms, phms) | ||
|
||
return hms | ||
|
||
def _get_predecessors(workflow, node): | ||
# recursive method to get predecessors of a given node | ||
pred = [] | ||
|
@@ -871,7 +881,7 @@ def _get_predecessors(workflow, node): | |
'artifact transformation'] | ||
merging_schemes = { | ||
qdb.archive.Archive.get_merging_scheme_from_job(j): { | ||
x: y.id for x, y in j.outputs.items()} | ||
x: str(y.id) for x, y in j.outputs.items()} | ||
# we are going to select only the jobs that were a 'success', that | ||
# are not 'hidden' and that have an output - jobs that are not | ||
# hidden and a successs but that do not have outputs are jobs which | ||
|
@@ -989,7 +999,7 @@ def _get_predecessors(workflow, node): | |
init_artifacts = { | ||
wkartifact_type: f'{starting_job.id}:'} | ||
else: | ||
init_artifacts = {wkartifact_type: self.artifact.id} | ||
init_artifacts = {wkartifact_type: str(self.artifact.id)} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hate to say it but having to cast an internal id to string is a little bit concerning to me. Internal code should use the numeric form of the artifact_id and it should also be easy to pass the numeric version for any REST call. May I ask why this is needed? I feel like even if it's needed, it would be much clearer to cast it right before it's used to make it obvious why it's needed. Sorry if that's not clear. |
||
|
||
cmds_to_create.reverse() | ||
current_job = None | ||
|
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.
The conversion to string for y.id is also not obvious here. Perhaps a comment as to why it's necessary would be appropriate?