-
Notifications
You must be signed in to change notification settings - Fork 0
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
ChIP-seq update to 2.1.6 #528
Conversation
…), updated wf version list
…hich work in other ff envs
…age in wfr_encode checks
(Apologies for the wonky Ubuntu commits) |
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've left a few non-blocking comments - there may be good reasons for doing as you have done and if the comments don't make sense feel free to ignore.
Dealing with the pairing situations are complex and given that you have done extensive testing I expect that if there is a list index used that is not present thereby causing an exception then that is indicating a definite problem that should cause the check to error - looking at spots like Line232 and 263 in wfr_encode_checks.py. Just wondering if a try/except to skip and report problematic cases might be warranted rather than having the whole check error?
@@ -195,9 +195,9 @@ def step_settings(step_name, my_organism, attribution, overwrite=None): | |||
}, | |||
{ | |||
"app_name": "encode-chipseq-aln-chip", | |||
"workflow_uuid": "4dn-dcic-lab:wf-encode-chipseq-aln-chip", | |||
"workflow_uuid": "212a9c91-25d6-473f-b56b-8dd93958c580", |
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.
despite the property name, might it be better to use the alias as was formerly done in case the workflow is added to a new environment (with a different uuid being generated) - perhaps unlikely but if at all possible?
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.
Yes, I was aiming to make it consistent with other workflows, since I made the new UUIDs consistent (for the current envs). The aliases also are the same for both new and old versions, which maybe also was a factor? It needs to know the new and old version are the same workflow, but only run the new one.
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.
Oops, I see your confusion--the aliases are definitely not the same and the app name keeps the versions together. I do think it is more consistent with other workflows to use the uuid. It is also hard to query aliases for someone going from this workflow setting to the workflow on the portal, right?
@@ -213,9 +213,9 @@ def step_settings(step_name, my_organism, attribution, overwrite=None): | |||
}, | |||
{ | |||
"app_name": "encode-chipseq-aln-ctl", | |||
"workflow_uuid": "4dn-dcic-lab:wf-encode-chipseq-aln-ctl", | |||
"workflow_uuid": "4eb427f1-a7d5-4d74-8cfa-4c77f42d5b43", |
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 as previous comment
@@ -226,9 +226,9 @@ def step_settings(step_name, my_organism, attribution, overwrite=None): | |||
}, | |||
{ | |||
"app_name": "encode-chipseq-postaln", | |||
"workflow_uuid": "4dn-dcic-lab:wf-encode-chipseq-postaln", | |||
"workflow_uuid": "291d4c64-75de-434a-9d98-01f40d19e15e", |
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.
see above
if organism == 'mouse': | ||
org = 'mm' | ||
input_files['chip.bwa_idx_tar'] = '/files-reference/4DNFIZ2PWCC2/' | ||
input_files['chip.bowtie2_idx_tar'] = '63e22058-79c6-4e24-8231-ca4afac29dda' |
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.
Again using uuid here could be prone to problems on different environments?
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.
Agreed on this one, though!
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.
Oh wait, this was also intentional (to resolve problems between envs). I will test to see if it's still an issue
@@ -158,15 +169,16 @@ def chipseq_status(connection, **kwargs): | |||
for merge_case in input_list: | |||
merge_enum += 1 |
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.
It may not matter but it could be confusing if this is incremented even though there is nothing in 'merge_case' - may want to move this after the if?
if organism == 'mouse': | ||
org = 'mm' | ||
s2_input_files['chip.blacklist'] = '/files-reference/4DNFIZ3FBPK8/' | ||
s2_input_files['chip.chrsz'] = '/files-reference/4DNFIBP173GC/' | ||
s2_input_files['chip.ref_fa'] = '/files-reference/4DNFIC1NWMVJ/' | ||
s2_input_files['chip.bowtie2_idx_tar'] = '63e22058-79c6-4e24-8231-ca4afac29dda' |
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.
again about using the uuid?
Modify wfr_encode_checks to run the updated (v2.1.6) ChIP-seq pipeline