GaNDLF
GaNDLF copied to clipboard
Convert main scripts to console entry points
Is your feature request related to a problem? Please describe. Currently, GaNDLF's main interactive components are based on standalone scripts, which are an older way of defining python packages.
Describe the solution you'd like Converting them to dedicated functions with console entry points would be better (a good read on why is here), and more compatible with future changes in python.
Describe alternatives you've considered N.A.
Additional context More details in: https://python-packaging.readthedocs.io/en/latest/command-line-scripts.html
Stale issue message
@sarthakpati - Can i take this issue up or work on a bit on it? I have already signed the form for github participation and I am in the working group
@sarthakpati - Can i take this issue up or work on a bit on it? I have already signed the form for github participation and I am in the working group
Thank you so much, and welcome! I have assigned this issue to you.
@sarthakpati - I have a doubt (sorry). I have finished going through the file structure etc.. I have one question - for example for GaNDLF.anonymize - I found this file. Is the intent of this ticket to just move/copy this code to command_line.py within anonymize/ and update the setup.py as entry_points={ "console_scripts": ["gandlf_anonymizer=GANDLF.anonymize:.console.main"] }
and then do this for all the interactive components. I have something for PR but I want to make sure I am on the right track before raising a PR
@sunny1401 - that's a great question. I personally feel it might be better for us to keep the algorithmic/functional components of the code separate from the portions of the code that deal with interfaces (which are current command-line only). Hence, perhaps putting this under GANDLF.cli
might be better, and then updating the setup.py
to:
entry_points={
"console_scripts": [
...
"gandlf_anonymizer=GANDLF.cli.anonymize:main",
...
]
}
This can similarly be extended for other such features that require command line interfaces, such as preprocessing:
entry_points={
"console_scripts": [
...
"gandlf_anonymizer=GANDLF.cli.anonymize:main",
"gandlf_anonymizer=GANDLF.cli.preprocess_and_save:main",
...
]
}
And so on. This can then be extended for graphical interfaces (if and when needed). What do you think?
Can you suggest a way to ensure the new code gets tested (currently, the cli scripts are getting skipped from code coverage)? The only thing that is coming to my mind is to change some of the existing tests to use the new interface(s). What do you think?
Can you suggest a way to ensure the new code gets tested (currently, the cli scripts are getting skipped from code coverage)? The only thing that is coming to my mind is to change some of the existing tests to use the new interface(s). What do you think?
I could add additional test and mock out sys params passing to test end to end functionality for the cli module?
Can you suggest a way to ensure the new code gets tested (currently, the cli scripts are getting skipped from code coverage)? The only thing that is coming to my mind is to change some of the existing tests to use the new interface(s). What do you think?
I could add additional test and mock out sys params passing to test end to end functionality for the cli module?
Sure, let's see how much longer these tests take. Perhaps we can find a way to combine it with some of the others.
Stale issue message
Stale issue message
Stale issue message
The "standard" way of cli interface is usually a cli command with sub-commands, like gandlf anonymizer --params
. What do you think if we support such an cli app / entrypoint? We may both support existing entrypoints (as you propose, gandlf_anonymizer
) for backward compatibility, as well as keep just one cli app.
That sounds awesome, thanks! 😄
If you are fine to work on this, then I will assign this to you, and we can proceed with your proposal. It would require us to update the documentation as well. But I think it would be a pretty big improvement WRT overall usability.
Fixed in #818