-
Notifications
You must be signed in to change notification settings - Fork 1
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(runner.py): new function added: implemented a function to download and extract the exomiser runner #74
base: main
Are you sure you want to change the base?
Conversation
…ad and extract the exomiser runner this function will handle the preinstallation processes for exomiser runner, and must exist for every different pheval runner ensuring a streamlined setup.
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 like the concept but there are some issues with the code:
- The code would benefit from being more modular, making it easier to maintain and for readability. For example, this could be wrapped up in a class separate from the runner class and then called within this install method. I would suggest this structure:
src/pheval_exomiser
├── __init__.py
├── cli.py
├── constants.py
├── install
│ ├── __init__.py
│ └── install_configurations.py
├── post_process
│ ├── __init__.py
│ ├── post_process.py
│ └── post_process_results_format.py
├── prepare
│ ├── __init__.py
│ ├── create_batch_commands.py
│ ├── tool_specific_configuration_options.py
│ ├── write_application_properties.py
│ └── yaml_to_family_phenopacket.py
├── run
│ ├── __init__.py
│ └── run.py
└── runner.py
and put all the code in the install_configurations.py
and call in the runner.
- Use docstrings in place of inline comments. Breaking the code up into smaller pieces as mentioned above will also remove the need for these comments.
I have one major question about this PR. How will this be implemented in the pipeline? Right now it is added as an install
method in the runner - but this will not be called in the run process as it is not known by PhEval. I don't think we want to make this compulsory for all new runners as it increases the burden on new developers. If we add this to the abstract class and nothing is implemented in the concrete runner will this throw an error? I am just wondering if this would benefit from being a separate command - but this does add another step.
One last question - where will this be all installed? In the input directory?
Hello, @yaseminbridges . I agree 100% with your points. This PR is just a "concept"; we need to discuss the major details. |
This function will handle the preinstallation processes for exomiser runner, and must exist for every different pheval runner ensuring a streamlined setup.
Impact: Enhances the installation workflow by automating the preparation of the Exomiser runner, improving efficiency and reducing manual steps