Skip to content
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

feat: enabled mbcs meta-tool in the ngs_mapping step model #545

Merged
merged 5 commits into from
Dec 6, 2024

Conversation

ericblanc20
Copy link
Contributor

Quick & dirty fix.

TODO: check & fix the whole pipeline (#528 )

@ericblanc20 ericblanc20 requested a review from tedil November 8, 2024 14:53
@ericblanc20 ericblanc20 linked an issue Nov 8, 2024 that may be closed by this pull request
@ericblanc20
Copy link
Contributor Author

The pull request CI testing is failing due to mamba configuration.

It seems to use miniconda instead of miniforge, and it seems that the new conda solver matches mamba's performance(?).
We should update our CI workflow (but I don't know how to do it...)

@ericblanc20 ericblanc20 marked this pull request as draft November 12, 2024 08:51
@ericblanc20 ericblanc20 marked this pull request as ready for review December 2, 2024 16:14
@coveralls
Copy link

Coverage Status

coverage: 85.77%. remained the same
when pulling 5319155 on 544-ngs_mapping-model-not-in-sync-with-code
into 6873ffa on main.

Copy link
Member

@tedil tedil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you! :)
The only comments I have are rather general ones which we need to discuss for snappy in its entirety, i.e. are not specific to this PR.

@@ -28,6 +28,6 @@ class HlaTyping(SnappyStepModel, validators.ToolsMixin, validators.NgsMappingMix

tools: Annotated[list[Tool], EnumField(Tool, [Tool.optitype], min_length=1)]

optitype: Optitype | None = None
optitype: Optitype = Optitype()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before I forget: This is a design decision we must make.

  • Either we always provide all the defaults, and substeps are always registered irrespective of whether they're used or not, or
  • we do not provide defaults but force None, in which case substeps must not be registered if the config doesn't provide the substeps and all functions in the workflow definitions (in the init.py files) must not access those non-existent config definitions (e.g. int get_output_files etc).

Copy link
Contributor Author

@ericblanc20 ericblanc20 Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am tempted to always provide defaults. It makes the __init__.py code easier, and I personally don't like to see:

step_config:
   hla_typing:
    tools: [optitype]
    optitype: {}

For me, the syntax optitype: {} suggests that the tool doesn't accept parameters, not that we use the defaults.
Also, I wouldn't always set all defaults for more complex steps/tools. If there isn't an obvious choice for a default, then the user should make a decision.

But again, I am prepared to be convinced otherwise.

Comment on lines +171 to +172
if self.name not in self.config.tools:
return {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also touches on the comment above

@@ -53,7 +53,7 @@ def pair_fastq_files(input_left, input_right):
input_left = snakemake.params.args["input"]["reads_left"]
input_right = snakemake.params.args["input"].get("reads_right", "")

config = snakemake.config["step_config"]["ngs_mapping"]["somatic"]
config = snakemake.config["step_config"]["ngs_mapping"]["mbcs"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for now, we can do it like this, and then later move those config accesses to the parameters

@ericblanc20 ericblanc20 merged commit 625187b into main Dec 6, 2024
6 of 7 checks passed
@ericblanc20 ericblanc20 deleted the 544-ngs_mapping-model-not-in-sync-with-code branch December 6, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ngs_mapping model not in sync with code
3 participants