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

Convert main scripts to console entry points #610

Closed
sarthakpati opened this issue Apr 3, 2023 · 14 comments · Fixed by #818
Closed

Convert main scripts to console entry points #610

sarthakpati opened this issue Apr 3, 2023 · 14 comments · Fixed by #818
Assignees
Labels
enhancement New feature or request

Comments

@sarthakpati
Copy link
Collaborator

sarthakpati commented Apr 3, 2023

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

@sarthakpati sarthakpati added the enhancement New feature or request label Apr 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Stale issue message

@sunny1401
Copy link
Collaborator

sunny1401 commented Jul 21, 2023

@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
Copy link
Collaborator Author

sarthakpati commented Jul 21, 2023

@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.

@sunny1401
Copy link
Collaborator

sunny1401 commented Jul 29, 2023

@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

@sarthakpati
Copy link
Collaborator Author

@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?

@sarthakpati
Copy link
Collaborator Author

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?

@sunny1401
Copy link
Collaborator

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?

@sarthakpati
Copy link
Collaborator Author

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.

@github-actions
Copy link
Contributor

Stale issue message

Copy link
Contributor

Stale issue message

Copy link
Contributor

Stale issue message

@VukW
Copy link
Contributor

VukW commented Feb 26, 2024

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.

@sarthakpati
Copy link
Collaborator Author

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.

@VukW VukW self-assigned this Feb 26, 2024
@sarthakpati
Copy link
Collaborator Author

Fixed in #818

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants