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

[Feature Request] Add auto_env_var_prefix #171

Closed
danielgafni opened this issue May 10, 2024 · 19 comments
Closed

[Feature Request] Add auto_env_var_prefix #171

danielgafni opened this issue May 10, 2024 · 19 comments
Assignees
Labels
enhancement New feature or request

Comments

@danielgafni
Copy link

danielgafni commented May 10, 2024

First of all, this project looks great!

I would love to replace Typer with cyclopts in my CLIs.
However, there is an important Click feature currently lacking in cyclopts: auto_env_var_prefix (called auto_envvar_prefix in Click).

Feature request

Allow setting auto_env_var_prefix in cyclopts.App (or app.command) which would generate a env_var for every parameter.

For example, this CLI command

from cyclopts import App

app = App(auto_env_var_prefix="MY_APP_")

@app.command
def foo(my_arg: int):
    print(my_arg)

could read the my_arg environment variable from MY_APP_MY_ARG.

This is extremely useful when building CLIs for deployment in various remote environments.

Nested commands should insert their names in the generated env variable.

@danielgafni danielgafni changed the title Feature request: auto_env_var_prefix [Feature Request] Add auto_env_var_prefix May 10, 2024
@BrianPugh
Copy link
Owner

BrianPugh commented May 10, 2024

I think we could/should support something like this! Does typer do any auto-namespacing for this? Or are all parameters just directly derived, regardless of the command hierarchy? In your example you have MY_APP_MY_ARG, I'm assuming we would not want MY_APP_FOO_MY_ARG, right? SEE EDIT BELOW.

What do we think about this proposed API:

app = App(
    default_parameter=Parameter(auto_env_var_prefix="MY_APP_"),
)

Design decisions and behavior:

  1. I think this configuration belongs in Parameter; it naturally lets the configuration inheritance mechanism do its thing. This also places it nicely next to Parameter.env_var.
  2. If both auto_env_var_prefix and env_var are supplied (either directly, or implicitly through configuration inheritance), env_var gets priority and auto_env_var_prefix is effectively ignored.
  3. The environment variable name will be derived something like:
cparam: cyclopts.Parameter  # Fully configured at this point

env_var = tuple(cparam.auto_env_var_prefix + cparam.name_transform(name).replace("-","_")upper() for name in cparam.name)
  1. If multiple explicitly specified long Parameter.name are provided, all of them will be used and multiple automatic env vars will be allowed. This will follow the rules of env_var.

  2. Default value of auto_env_var_prefix: Optional[str] will be None (deactivated). An empty string is a valid value to enable the feature, resulting in env vars without a prefix like MY_ARG.


Misc open-ended questions:

  1. Any thoughts/opinions on the above?

  2. Do we want to call it auto_env_var_prefix, or one of {env_var_auto_prefix, env_var_prefix_auto}. Benefits of the former is that it reads nicely and directly mirrors Typer. Benefits of the latter are that it makes all the env_var attributes start with env_var, which helps keeps things a bit more organized.


EDIT: Sorry, just noticed your comment in your initial post:

Nested commands should insert their names in the generated env variable.

So then to confirm, the following command:

@app.command
def foo(my_arg: int):
    print(my_arg)

Should actually have env var MY_APP_FOO_MY_ARG, correct?

Something like

@app.default  # NOTICE: DEFAULT
def foo(my_arg: int):
    print(my_arg)

will result in MY_APP_MY_ARG.

@danielgafni
Copy link
Author

danielgafni commented May 10, 2024

  1. Agree
  2. I think we should do a union instead, it's bad to accidentally confuse the user by not utilizing some of the provided variables. We can just combine both options and check them all.
    This pattern empowers having obvious environment variables for all parameters with clear naming, but also doing aliases for parameters with complex, long or unreadable names.
  3. Agree
  4. Agree
  5. Agree

Re: naming - I don't think we are bound to Typer's naming.

env_var_auto_prefix fits the rest of the parameters better.


Re: edit - correct

@BrianPugh
Copy link
Owner

Can you elaborate on 2? I might be a bit confused.

Regardless, I can begin working on this once I'm done with #165

@BrianPugh
Copy link
Owner

So actually, this might play super nicely with #165 as a config. It would look something like:

app = cyclopts.App(
    # Multiple configs are allowed, incase you also want to use a toml or something.
    config=cyclopts.config.Env("MY_APP_", command_namespace=True), 
)

@danielgafni
Copy link
Author

I mean that if an env_var is set in both Parameter and App level, the argument should try to read it's value from both environment variables. From both MY_APP_MY_ARG and MY_CUSTOM_ENV_VARIABLE.

@BrianPugh
Copy link
Owner

oh gotcha! Yeah I think that makes sense. The Parameter.env_var will be attempted to be read from first, then the automatic ones.

I almost have #165 in a good state, I'll be going with the approach mentioned above. I should have something in a few days.

@danielgafni
Copy link
Author

Alright, great! Thank you for your work!

@BrianPugh
Copy link
Owner

Just an update on this: I haven't forgotten about it. I've been moving and that's been sucking up a lot of my time 😅

@danielgafni
Copy link
Author

Completely understand, moving right now myself, can relate

@BrianPugh
Copy link
Owner

This feature is implemented in the newly released v2.7.0. See the example in the docs and the API docs for usage.

@danielgafni
Copy link
Author

@nordmtr fyi

@danielgafni
Copy link
Author

Hey @BrianPugh, thanks for implementing this!

I have just a small question.

I just took a look at the docs and realized the current implementation of this feature is mutually exclusive with the .toml config.

I wonder if it's necessarily should be the case? Shouldn't the user be able to use both at the same time? I mean defining some arguments in the .toml config, and some with environment variables (they should take precedence).

@BrianPugh
Copy link
Owner

you can! simply provide a list of callables to config. E.g.:

app = cyclopts.App(
    name="character-counter",
    config=[
        cyclopts.config.Env(
            "CHAR_COUNTER_",  # Every environment variable will begin with this.
        ),
        cyclopts.config.Toml(
            "pyproject.toml",  # Name of the TOML File
            root_keys=["tool", "character-counter"],  # The project's namespace in the TOML.
            # If "pyproject.toml" is not found in the current directory,
            # then iteratively search parenting directories until found.
            search_parents=True,
        ),
    ],
)

Here the environment variables will get precedence since they are defined earlier in the config list.

@danielgafni
Copy link
Author

Oh, thanks, didn't realize this was a thing.

@NodeJSmith
Copy link

@BrianPugh

I just spent about 20 minutes trying to figure out why this wasn't working and I think this is the answer, but it's not very intuitive.

What I was trying to do was something like this:

app = App(config=cyclopts.config.Env("OTF_"))

OPT_TOKENS = Annotated[str, Parameter(show=False, allow_leading_hyphen=True)]
OPT_USERNAME = Annotated[str, Parameter(help="Username for the OTF API")]
OPT_PASSWORD = Annotated[str, Parameter(help="Password for the OTF API")]
OPT_LOG_LEVEL = Annotated[str, Parameter("CRITICAL", help="Log level", env_var="LOG_LEVEL")]

@app.meta.default
async def launcher(*tokens: OPT_TOKENS, username: OPT_USERNAME, password: OPT_PASSWORD):
    api = await Api.create(username, password)
    command, bound = app.parse_args(tokens)
    return command(*bound.args, **bound.kwargs, api=api)

@app.command
def test(*,api:Annotated[Api,Parameter(parse=False)]):
    print("test")

But it seems multiple things go wrong here. This doesn't seem to work with meta, as far as I can tell. It also doesn't seem to work with default, regardless if it's meta or not. I've only gotten it to work by changing it to @app.command.

I got this to work the way I wanted eventually by just setting the env_var on the parameter directly, like env_var=["OTF_EMAIL","OTF_USERNAME"], and that works in the meta and in the default, without issue. I think this may need to be clearer in the documentation.

@BrianPugh
Copy link
Owner

investigating; thanks for the report!

@BrianPugh
Copy link
Owner

Quickly testing the following python script:

import cyclopts
from cyclopts import App

app = App(config=cyclopts.config.Env("OTF_"))

@app.default
def main(foo="python-default"):
    print(foo)

if __name__ == "__main__":
    app()
$ python issue171.py
python-default

$ python issue171.py cli-value
cli-value

$ OTF_FOO=env-value python issue171.py
env-value

So it certainly works with @app.default. However, I am running into issues with meta. Investigating deeper.

@BrianPugh
Copy link
Owner

Opened #184 which should address the unintuitive behavior.

@BrianPugh
Copy link
Owner

Bit of unsolicited advice, but I'd recommend using docstrings to minimize the number of parameters you need to Annotated:

import cyclopts
from cyclopts import App, Parameter
from typing import Annotated


app = App(config=cyclopts.config.Env("OTF_"))


@app.meta.default
async def launcher(
    *tokens: Annotated[str, Parameter(show=False, allow_leading_hyphen=True)],
    username: str,
    password: str,
):
    """My OTF CLI.

    Parameters
    ----------
    username: str
        Username for the OTF API.
    password: str
        Password for the OTF API.
    """
    print(f"{username=} {password=}")
    command, bound = app.parse_args(tokens)
    return command(*bound.args, **bound.kwargs, api=None)


@app.command
def test(*, api: Annotated[str, Parameter(parse=False)]):
    print("test")


if __name__ == "__main__":
    app.meta()

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
Development

No branches or pull requests

3 participants