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

Replace argparse CLI with typer. #606

Open
pfebrer opened this issue Aug 3, 2023 · 5 comments
Open

Replace argparse CLI with typer. #606

pfebrer opened this issue Aug 3, 2023 · 5 comments

Comments

@pfebrer
Copy link
Contributor

pfebrer commented Aug 3, 2023

I would like to propose to rewrite sisl CLIs with typer instead of argparse.

Why?

Because:

  1. It autogenerates CLIs for functions using typehints. This has several advantages:
    • We can use sisl functions directly for CLIs, without needing to redefine them and their arguments.
    • To do so, we are forced to type these functions clearly, which is good :)
  2. It looks nice out of the box using rich, which uses colors in the terminal 🎨

Thanks to the first point, I envision we could implement commands like sgeom without all the boilerplate code that there is currently in the Geometry class. In fact, we could automatically create a command for any class that we want in sisl which would include all its methods. E.g.:

sisl density-matrix mulliken ...mulliken options...

Then, all arguments would automatically work without needing to describe the interface separately, as it is done with argparse, which means that it will be much easier to maintain a complex CLI.

What would be needed?

We would need to define a parser for any custom type that might appear in sisl's type hinting if we want to support it. Say for example, if we have a function that receives a geometry:

def does_something(geometry: sisl.Geometry):
     return ...

of course you can't pass a geometry through the CLI. You would then define a parser:

def parse_geometry(geometry: str):
    return sisl.Geometry.read(geometry)

and pass it to typer whenever there is a Geometry annotated function. Typer accepts this kind of parameter options as metadata of Annotated:

def does_something(geometry: typing.Annotated[sisl.Geometry, ...metadata...]):
     return ...

so we would need to define a wrapper that introduces this kind of information before passing it to the typer CLI. But it would work for ALL functions. Another thing that goes into the metadata is the help message of the parameter. It would be retrieved from the docstring. Since all docstrings in sisl follow the same standard, it is quite easy.

I have already implemented the proof of concept wrapper and it works nicely.

I think this would standarize sisl CLIs, making them more familiar to the users, and can facilitate creating new CLIs in the future (e.g. for plotting 😄). If you agree, I could give it a try to reproduce the existing CLIs and create a PR 👍

@zerothi
Copy link
Owner

zerothi commented Aug 5, 2023

What about Click?

There are many other alternatives, although I am quite fond of rich.

One thing is that the current CLI is doing stuff depending on the order of flags, do they automatically do this?

I'll think some more and come with some more comments.

@pfebrer
Copy link
Contributor Author

pfebrer commented Aug 5, 2023

Typer is built on top of click and you can customize a typer CLI with click concepts.

With "raw click" I find the syntax for defining arguments as verbose as argparse, so I don't particularly like it. I like the typer approach because I find specifying things through type hints to be beneficial for many other aspects: documentation, creating APIs, other CLI frameworks... So it feels natural vs writing code that it's only specific to a certain CLI framework. But of course if we are going to write a wrapper to implement some custom behavior anyways this is a weaker point.

I think the behavior of sgeom wouldn't be something that is straightforward to implement, because the flags are not really options and therefore they can not be related directly to the arguments of a function. For example the same flag can appear twice in the same command. Typer has automatic support for chaining subcommands though. So each method of the Geometry class would be a subcommand of sgeom and you can call them sequentially in the same go. This I believe is more natural, but it would change the paradigm of the CLI though. Of course if you want to stick with using flags to indicate operations, that would be still possible because typer allows you to avoid default parsing and parse the arguments however you want. So probably it's not very difficult to do.

@pfebrer
Copy link
Contributor Author

pfebrer commented Aug 8, 2023

We could start with the sisl_toolbox CLI, which is much simpler and not that crucial and see how it goes :) I can submit a PR with the sisl_toolbox CLI converted to typer if you are willing to check whether you like it or not, it won't be too much work I think.

Sorry to rush this but I have the basis optimization CLI ready to merge but for the moment it's in typer so I would like to know if it can be kept in typer or not 😄

@pfebrer
Copy link
Contributor Author

pfebrer commented Aug 8, 2023

My idea is to create a CLIargs dataclass that contains all parameters that a CLI framework might use and add it as metadata in the type hint using Annotated:

from typing_extensions import Annotated
from somewhere_in_sisl import CLIargs

def my_sum(
    first: Annotated[int, CLIargs("-f", metavar="Whatever")],
    second: Annotated[int, CLIargs("-s", metavar="Another")]
):
    """Sums two numbers
    
    Parameters
    ---------------
    first:
         First argument.
    second:
         Second argument.
    """
    return first + second

The CLI would be created from the functions' signature (including type hints and metadata) and the docstring. Then changing from one CLI framework to another would be just a matter of changing the function that creates the CLI. So I would be taking from typer the idea of using type hints, which I think is the way to go, but not directly annotating with typer classes so that other CLI frameworks can be used to create the CLI easily.

@zerothi
Copy link
Owner

zerothi commented Aug 8, 2023

Sounds like a plan.

Here would be my proposal:

  1. You create a PR with a single of the tools converted into a typer mechanism, I would suggest siesta.atom or transiesta.poisson, this should be a minimal PR.
  2. Once this is cleared we can move all of the tools into typer mechanism
  3. Then your new things could be added in a 2nd PR, being orthogonal means simpler PR's, and easier for me to grasp.
  4. I would like to wait a bit with the other CLI, there are so many things in the new release that I really don't want to force push anything else, I would prefer to keep things rather stable, with some planned additions.

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

No branches or pull requests

2 participants