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

GeneSplicer: improve argument processing #689

Merged

Conversation

nuno-agostinho
Copy link
Contributor

@nuno-agostinho nuno-agostinho commented Feb 2, 2024

Improves on Ensembl/ensembl-vep#1599

Changelog

  • Accept genesplicer command instead of a path
    • Works if genesplicer is exported in the $PATH variable (like in VEP Docker)
    • Path to genesplicer is internally obtained via which
  • Add key-value params to first two positional arguments:
    • binary with genesplicer as default
    • training with no default
  • Improve plugin docs
    • Add information on how to use GeneSplicer with VEP Docker/Singularity

Testing

If genesplicer is exported in the $PATH variable, like in VEP Docker/Singularity (Ensembl/ensembl-vep#1599), there is no need to explicitly call it:

vep --id rs1348242192 --database --force --plugin GeneSplicer,training=${human_data}

This means that the only mandatory argument now is the training data.

@nuno-agostinho nuno-agostinho changed the title GeneSplicer: accept command as argument GeneSplicer: improve argument processing Feb 2, 2024
@nuno-agostinho nuno-agostinho marked this pull request as ready for review February 2, 2024 16:35
Copy link
Contributor

@olaaustine olaaustine left a comment

Choose a reason for hiding this comment

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

Hi @nuno-agostinho,
Thank you very much for this change, tested while running on docker v4.27.1
Just some changes.
Thank you
Ola

GeneSplicer.pm Outdated Show resolved Hide resolved
GeneSplicer.pm Outdated Show resolved Hide resolved
GeneSplicer.pm Outdated Show resolved Hide resolved
@nuno-agostinho
Copy link
Contributor Author

Hi @olaaustine, I updated the PR with your suggestions. Thanks!

Copy link
Contributor

@olaaustine olaaustine left a comment

Choose a reason for hiding this comment

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

Hi @nuno-agostinho,
Thank you for this change.
Tested on Docker and on the cluster.
LGTM!.
Waiting for this before merging.
Thank you

@olaaustine olaaustine merged commit 047fc6c into Ensembl:postreleasefix/112 Feb 19, 2024
@olaaustine
Copy link
Contributor

Merged into release/112 and main.

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.

2 participants