feat: configure active workflows with stepper inputs #662
Conversation
|
will this supersede #570?? |
|
I suppose that can happen at any point since the workflows we're updating there aren't active |
mvdbeek
force-pushed
the
extend_param_config
branch
from
85fa197 to
ecbfbbb
Compare
I don't know how to properly assign the pools. Are those separate run accessions ? Similar issue with the single cell CMO workflow that requires two PE collection inputs, not sure how that is deposited and retrieved from SRA / ENA.
we might not need fastq data
mvdbeek
force-pushed
the
extend_param_config
branch
2 times, most recently
from
7034332 to
ec0b097
Compare
mvdbeek
force-pushed
the
extend_param_config
branch
from
ec0b097 to
9dc1bea
Compare
mvdbeek
changed the title
mvdbeek
force-pushed
the
extend_param_config
branch
from
9dc1bea to
d699255
Compare
|
This should be a nice enhancement. Not sure that the single / paired split for the ENA config is the best way to go about it. We'll have to eventually pass in props for organism, read type (short read vs long read), single-paired etc. that should control the default api filters we're using, so maybe passing along some kind of config object is a better idea. |
|
holy guacamole batman! hold please.. i need to go grab my thinking cap and put on some much higher energy music for this 😂 |
There was a problem hiding this comment.
a huge improvement! seriously, thanks @mvdbeek 🦸
Not sure that the single / paired split for the ENA config is the best way to go about it. We'll have to eventually pass in props for organism, read type (short read vs long read), single-paired etc. that should control the default api filters we're using, so maybe passing along some kind of config object is a better idea.
i agree, but think all we need to do about it right now is file an issue and reference it here.
| geneModelUrl?: string | null; | ||
| readRuns?: EnaPairedReads[] | null; | ||
| readRunsPaired?: EnaPairedReads[] | null; | ||
| readRunsSingle?: EnaPairedReads[] | null; |
There was a problem hiding this comment.
is EnaPairedReads actually specific to paired reads then? it seems like we either need something else here for readRunsSingle, or we should renmame EnaPairedReads ??
There was a problem hiding this comment.
I wanted to limit the refactoring here, but you're right the interface is only
export interface EnaPairedReads {
md5Hashes: string;
runAccession: string;
urls: string;
}
which is generic enough for single and paired reads. I'll rename it to EnaSequencingReads
| } | ||
|
|
||
| // Narrow type specific to the one kind of collection we use -- might be worth defining a more general collection type if we need other kinds of collections | ||
| // Narrow type specific to the two kinds of collections we use -- might be worth defining a more general collection type if we need other kinds of collections |
There was a problem hiding this comment.
the relationship between these two things could be clarified. meaning, a lot of places were using single vs paired language but not here, and technically the paired one is also a list no? it leaves the names of the interfaces alone slightly confusing, until you think about it from galaxy perspective instead of just the context of this pr. i understood on my second read through, but still, even if all we do is improve the comment above.. future danielle's brain would appreciate it
There was a problem hiding this comment.
Single vs Paired only makes sense on the BRC side where we match reads to input parameters, paired is a special case of collection type. You might as well express it as list:list, where the inner list has only 2 items, forward and reverse. I think this is OK as a comment ? Or I don't know what to improve here
| import { PAPER_PROPS } from "./constants"; | ||
| import { useEffect } from "react"; | ||
| import { STEP } from "../../step"; | ||
| import { PAIRED_END_STEP } from "../../step"; |
There was a problem hiding this comment.
this has been a lot to digest so maybe i didnt read it right, but this needs to be a bit cleverer no? or it thinks its not configured for single end workflows?
There was a problem hiding this comment.
That's a good catch, this is why you can't bypass the ENA step in single end workflows
mvdbeek
force-pushed
the
extend_param_config
branch
2 times, most recently
from
5b57205 to
d1c346b
Compare
mvdbeek
force-pushed
the
extend_param_config
branch
from
d1c346b to
0810f94
Compare
There was a problem hiding this comment.
Hey @mvdbeek! I noticed that the STEP constant and LaunchStep might not be used elsewhere in the codebase. Just wanted to check if these are still needed, or if they might be leftover code debt?
3 participants
builds on #660 and #661.
Some of these workflows don't consume fastqs or GTFs or both, so I also implemented
#652 and some of these need single end collections, so I implemented #659
closes #580, #659, #568