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? Customizable prefixes for boolean options. #171

Closed
emcd opened this issue Oct 5, 2024 · 5 comments
Closed

Feature? Customizable prefixes for boolean options. #171

emcd opened this issue Oct 5, 2024 · 5 comments

Comments

@emcd
Copy link
Contributor

emcd commented Oct 5, 2024

A typical pattern for configuration of an application is to have configuration from files (or defaults), which can be overridden by environment variables, which can be overridden by command-line arguments. For boolean configuration values, this implies that the absence of an environment variable or command-line argument, corresponding to the underlying configuration variable, should be treated as honoring the configuration from files or defaults. While Tyro does provide for optional flags (True/False/None), which can model this, the semantics can sometimes be a little weird.

For example, suppose that I have an AI workbench app which allows for me to enable or disable all vector stores available to it. Finding a good name for the flag which enables all vector stores, when True, disables all vector stores, when False, and defers to the underlying configuration, when None, is somewhat problematic. If it is called all-vectorstores-enabled or all-vectorstores-enablement, then it is not clear that False actually disables all of them. A more natural way to express this would be to have a single flag with a pair of names: --enable-all-vectorstores and --disable-all-vectorstores. If the flag is not provided on the command line, then it has a value of None (i.e., defer to the environment variables, configuration files, or defaults).

What do you think? Too much of an edge case to be worth introducing additional complexity? Or something worthy of consideration?

(Currently, I am using a "tristate" enum, which has members enable, retain, disable, with flags like --all-vectorstores, where retain is the default, but this feels less natural than what I have proposed above.)

@brentyi
Copy link
Owner

brentyi commented Oct 11, 2024

Hi @emcd!

In general I think using an enum or Literal["enable", "retain", "disable"] seems reasonable for this use case but yeah, I can see why you'd want something better.

For the sake of maintainability I'd prefer not to veer too far away from the default things supported by argparse. Perhaps one way to accomplish what you're looking for, however is argparse's mutually exclusive groups? The usage: PROG [-h] (--foo | --bar) example seems pretty close to what you're looking for. This is something that I've considered for a while but just hasn't happened because I haven't thought of an API that I'm happy with. If you have any suggestions or thoughts I'd be interested!

@emcd
Copy link
Contributor Author

emcd commented Oct 11, 2024

Hi @brentyi. Interesting idea. I am not sure how you would model the "retain" (do nothing / use configuration) state with the mutually-exclusive groups, assuming that they are backed by a shared boolean variable. What would not specifying either mutually-exclusive flag mean? And, from a developer standpoint, I would prefer to model these tristate cases with one argument specification rather than two argument specifications behind the scenes... better maintainability of the code that uses Tyro.

In any case, not a big deal. Just wanted to float this idea to get your thoughts. I respect your desire to not wander too far beyond the standard argparse capabilities.

@brentyi
Copy link
Owner

brentyi commented Oct 11, 2024

Sorry, to clarify I think staying close to default argparse things isn't in confict with this feature request. I'm thinking it's possible to have a single Python field that gets converted to three mutually exclusive Python arguments. Like one of:

vectorstore_mode: Literal["all", "retain", "disable"]
vectorstore_mode: Literal[True, False] | None
vectorstore_mode: bool | None

Could be converted to:

--all-vectorstores  # set to `False` by default
--retain-vectorstores  # set to `True` by default
--disable-vectorstores  # set to `False` by default

Which are marked in argparse as mutually exclusive (only one is allowed to be passed in).

I think this is reasonable, I'm just not sure about what the right syntax would be for tyro to do this conversion. It's also possible the implementation complexity isn't worth it—generally tyro is made for developer convenience and not fine control over CLI details—but that isn't obvious to me yet.

@emcd
Copy link
Contributor Author

emcd commented Oct 11, 2024

Ah - I see. Yes, I think that converting to three mutually-exclusive options behind the scenes is reasonable. From a user experience perspective, I would prefer if --retain-all-vectorstores was not even visible though. --enable-all-vectorstores / --disable-all-vectorstores is all a user is going to care about / understand. The --retain-all-vectorstores option is essentially interface clutter which can potentially add confusion, when presented as a selectable behavior - even though it is the desired default behavior.

(The real world use cases motivating this are actually a couple of vectorstores-related issues. The app that I'm developing broke when Langchain refactored its vectorstores API sometime in late 2023 or early 2024. And, it also broke when Chroma changed their backing store across minor versions last year. At the time, I didn't have a CLI so I had to comment out the vectorstore configs. Would have been nice to do --disable-all-vectorstores --enable-vectorstore faiss-github-docs (ignoring the Chroma-based vectorstores and using a particular FAISS-based vectorstore) on the command line then. And I want to be able to do this naturally in the future.)

Honestly, what Tyro provides right now with bool | None is most of the way there. What's "missing", imo, is the ability to control the names and aliases of the flags generated from that. Something like be ability to specify a tuple of strings instead of a string.... But, my limited exposure to the Tyro code base makes my think that that would add a fair amount of complexity to it. I would understand if your answer is "no".

@brentyi
Copy link
Owner

brentyi commented Oct 12, 2024

Got it, I appreciate the explanation.

Yeah, after thinking about it more the complexity does seem quite high; unfortunately I also won't have much time over the next few months for my "for fun" projects like tyro. So I'll close this for now. But if any ideas occur to you for what a feasible API for this might look like feel free to re-open or make another issue. 🙂

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