-
Notifications
You must be signed in to change notification settings - Fork 3
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
(Pipeline Integration) Add starter factory #4295
base: dev-start-pipelines
Are you sure you want to change the base?
Conversation
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.
Unclarities added
seqera_platform_client: SeqeraPlatformClient = self._get_seqera_platform_client() | ||
return SeqeraPlatformSubmitter( | ||
client=seqera_platform_client, | ||
compute_environment_ids=self.cg_config.seqera_platform.compute_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.
This reaching through the SeqeraPlatformConfig indicates that our server config is not designed properly.
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.
What if we have a method like this?
def _get_seqera_platform_submitter(config: SeqeraPlatformConfig):
client = SeqeraPlatformClient(config)
return SeqeraPlatformSubmitter(client=client, compute_environemnt_ids=config.compute_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.
It is more the fact that the computeEnvIds are not needed by the client at all anymore, but they are set in the config we designed for the client. I guess we should have one config for the submitter and one for the client?
params_file_creator: ParamsFileCreator = RarediseaseParamsFileCreator( | ||
lims=self.cg_config.lims_api, store=self.store, params=pipeline_config.params | ||
) | ||
sample_sheet_creator: RarediseaseSampleSheetCreator = RarediseaseSampleSheetCreator( |
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.
Clearly these should be initiated in a dynamic way and not only be Raredisease do we know if the inits are the same? What do we need to pass to fetch the correct 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.
I'm 99% sure the signatures of other NF pipelines will be the same
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.
You can make a function that pulls the correct one from a map for example. workflow_to_samplesheet_creator (so pass the workflow)
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.
Yeah, that might be the nicest approach but then we need to fix the init in the parent NextflowSampleSheetCreator class.
pipeline_extension=self._get_pipeline_extension(workflow), | ||
) | ||
|
||
def _get_pipeline_config(self, workflow: Workflow) -> CommonAppConfig: |
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 fact that all the nextflow configs inherit only from CommonAppConfig leads to our code being riddled with warnings because of unknown attributes.
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.
We need to create a separate basemodel to inherit from IMO
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 don't think it looks too bad 😁
workflow_config_path=pipeline_config.workflow_config_path, | ||
) | ||
params_file_creator: ParamsFileCreator = RarediseaseParamsFileCreator( | ||
lims=self.cg_config.lims_api, store=self.store, params=pipeline_config.params |
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.
what are the params here?
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 is what the devbio have named the pipeline parameters file path that you can find in the cg.yaml
configurator: Configurator = self._get_configurator(case_id) | ||
input_fetcher: InputFetcher = self._get_input_fetcher(case_id) | ||
submitter: Submitter = self._get_submitter(case_id) |
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.
These all take the case_id as input but they all just depend on the workflow. And some of the smaller functions take workflow as input. Do you think it makes sense to just pass the workflow here as well?
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.
My thinking initially was that the input fetcher depends on the delivery type, but then I remembered that most delivery types are not only fastq or bam, so at this point, yeah - we should probably pass the workflow.
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.
Unless there is more information required than just the workflow it makes sense, if it turns out later you need more information than case_id will allow to fetch more information of course. I'd wait with changing it until it is more final.
params_file_creator: ParamsFileCreator = RarediseaseParamsFileCreator( | ||
lims=self.cg_config.lims_api, store=self.store, params=pipeline_config.params | ||
) | ||
sample_sheet_creator: RarediseaseSampleSheetCreator = RarediseaseSampleSheetCreator( |
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'm 99% sure the signatures of other NF pipelines will be the same
pipeline_extension=self._get_pipeline_extension(workflow), | ||
) | ||
|
||
def _get_pipeline_config(self, workflow: Workflow) -> CommonAppConfig: |
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.
We need to create a separate basemodel to inherit from IMO
seqera_platform_client: SeqeraPlatformClient = self._get_seqera_platform_client() | ||
return SeqeraPlatformSubmitter( | ||
client=seqera_platform_client, | ||
compute_environment_ids=self.cg_config.seqera_platform.compute_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.
What if we have a method like this?
def _get_seqera_platform_submitter(config: SeqeraPlatformConfig):
client = SeqeraPlatformClient(config)
return SeqeraPlatformSubmitter(client=client, compute_environemnt_ids=config.compute_environments)
store=self.store, | ||
workflow_config_path=pipeline_config.workflow_config_path, | ||
) | ||
params_file_creator: ParamsFileCreator = RarediseaseParamsFileCreator( |
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.
These likely needs it own function as soon as there are more than the "RareDiseaseParamsFileCreator"
|
Description
Added
Changed
Fixed
How to prepare for test
us
paxa
How to test
Expected test outcome
Review
Thanks for filling in who performed the code review and the test!
This version is a
Implementation Plan