-
Notifications
You must be signed in to change notification settings - Fork 24
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
Multiple nested optional settings #60
Comments
Thanks for filing this @jordanott! I just pushed a new release ( Unfortunately due to positionality limitations of the subcommand mechanisms we're relying on here, however, your first example ( |
Thanks for the speedy reply and fix!
Any specific reason the subcommand mechanism does not respect the default argument and use that? |
This is one of a handful of remaining things in For union types over structures like dataclasses, subparsers make sense because they let us only accept arguments from the chosen types. The downside is that each parser or subparser can only contain a single set of "child" subparsers. This results in a tree structure that doesn't map exactly to the case where we have multiple Union types. In your case, the tree structure we generate looks like:
...where Of course open to suggestions here. :) The best I've been able to come up with is to tear out |
This is also related to a discussion on the semantics of Since The correct semantics here would be: not creating subcommands, but an "optional" field which resolves to |
Are you arguing that the annotation { I can see why creating a subcommand isn't convenient for all use cases, but don't see why it's necessarily wrong; staying consistent with the semantics of For better or worse it also follows
|
No, I was thinking about the case Sorry for a very long post, I write this very specifically to make things clearer and easier to understand for other people. TL;DR) The current behavior seems a bit buggy, and the behavior of I was thinking that we may have two possible design choices on the behavior of
but now I see that creating a subcommand is not necessarily excluding nested arguments. To clarify: please note that when I said ExamplesLet's discuss a more concrete example with the code (a simplified version of the OP's code). 0. Without
|
We currently generate subcommands/subparsers whenever a type needs to be selected for struct-like/dataclass fields, since they let us generate mutually exclusive groups of arguments. Does the reasoning for this makes sense for from dataclasses import dataclass
import tyro
@dataclass
class Dataclass1:
content: int
"""Integer content."""
@dataclass
class Dataclass2:
content: str
"""String content."""
def main(x: Dataclass1 | Dataclass2) -> None:
...
tyro.cli(main) Instead, we have access to each variant via For I understand there are many downsides here associated with subcommands, particularly things like:
But these tradeoffs were ones we were aware of when making the current design decisions. They're also why options like
The current "Perhaps you meant" message should say: the intention is to communicate that the argument could be misplaced, especially because it exists in a different parser. (like If you have suggestions for how to make this message clearer I'd be interested!
To clarify, the
Yes: since the Question: we could make this simpler by not creating subcommands for
Okay, I'm glad this sort of makes sense. I agree that the syntax is inconvenient and verbose, but the decision to make this the default behavior still makes sense to me. Particularly when we have multiple unions: @dataclasses.dataclass(frozen=True)
class Config:
optimizer1: Optional[Optimizer] = Optimizer()
optimizer2: Optional[Optimizer] = Optimizer() we would create the subcommands It can always be shortened with
One nice thing about the current subcommand behavior is that this case is well-defined and consistent with how we handle [1.] and [2.]. If there's no default value, either |
Hi @brentyi, This library is fantastic. We are planning on using tyro in conjunction with pydantic settings for additional environment support and validation. They are a perfect fit 😄 Outside of the discussion on Resolving the below question would be a large value add to the library:
For
I completely agree with subcommands for unions that include two or more types that are not However, for an My guess is, the challenge for implementing a solution likely relates to how exclusivity and subparsers are handled. I suspect if we keep the focus narrowed to just I would be more than happy to jump in and help if pointed in the right direction 👍🏾 |
Thanks for your thoughts + offering to help @kschwab! I'd certainly appreciate any efforts to improve the ergonomics here. If you want to get started, I'm happy to provide feedback if you can iron out more behavior details + maybe even unit tests. Some misc things I'm wondering: @dataclass
class Optimizer:
lr: float = 1e-4
def main(optimizer: Optimizer | None) -> None:
...
if __name__ == "__main__":
tyro.cli(main)
If you want to take a stab at implementing some changes, I'd recommend breaking things down into two main steps:
|
Awesome, thanks @brentyi. Yes, I will work to pick this up after getting your thoughts on best direction.
Correct. I believe that would be the most intuitive.
This was the main distinction between option 1 and the others. If we go with
Also, just as with pydantic settings, any nested flags would then take precedence. So a larger example could look like:
However, in pydantic you have the full backing of the library to sort out the details and validate the JSON string. Since tyro is general, I'm not sure if we'll run into trouble or how much of a hassle it would be (hence the other two options that limit it to more of a flag). Alternatively, we could also take the approach of your next question and basically limit it to
This is also taking a page out of the pydantic settings book with source prioritization. They apply deep updates on their sets based on the source order. For tyro, ideally this would mean the initial set is sourced from "init settings", i.e.
Not familiar enough with this one. Could you help provide an example?
This question actually made me lean more towards options 2 or 3. For sure this should result in a subcommand since we have two or more types that are not
I personally don't have much of an issue with the above, it's very reasonable. This is why the proposal was only for
Which would also blend in nicely for the case we're targeting, e.g.
However, I think if we're serious about tackling |
Yeah, I think this would be a fairly large hassle. It might be a different situation if we already used JSON somewhere, but this issue (special-casing a specific class of
Yes, we do have some similar prioritization logic! To rephrase what's at the root of my questions here though, my assumption is if you run this script with no arguments: # script.py
@dataclass
class Optimizer:
lr: float = 1e-4
def main(optimizer: Optimizer | None) -> None:
...
if __name__ == "__main__":
tyro.cli(main) that it should throw an error, since there's no information about what the default value of In this case, what's the mechanism for choosing that you want to set
Sure, I just meant quite literally that instead of running: def main(optimizer: Optimizer | None) -> None:
...
if __name__ == "__main__":
tyro.cli(main) we should also be able to run just: if __name__ == "__main__":
tyro.cli(Optimizer | None) the same way we can run
Could you actually elaborate on this? I don't fully follow the relevance of #61. Thanks again! |
@brentyi thanks for the clarification, understood now on the below case: if __name__ == "__main__":
tyro.cli(Optimizer | None) i.e., a "required" I would be in favor of resolving this case with some "is_strict_mode" flag into tyro, with the default being "lax". In summary, after the discussions above, I would propose the below: 1.
|
Also, with respect to
|
Thanks for writing that up! The details were really helpful for understanding what you're thinking.
I'd prefer the behavior of keeping In terms of consistency this would also match Python semantics and the current If it would make things easier we could also consider changing On For example, if we have: @dataclass
class Level3:
...
@dataclass
class Level2:
level3: Level3 | None
@dataclass
class Level1:
level2: Level2
On the matter of whether But unfortunately there are cases where we will still need a subcommand corresponding to @dataclass
class Args:
a: T1 | T2 | None
b: T1 | T2 | None
tyro.cli(Args) The subcommands for field
or do some flattening:
This example also has me changing my mind on the relevance of #61... but this is challenging to implement and also a mixed bag to me. It would need to be configurable; there are several existing projects running As one solution for simplifying everything, another option is to narrow the scope of the PR even further. In particular, we could introduce the proposed |
Thanks for the feedback! Reading the above, we have great alignment.
Agreed. Point taken, that makes sense.
Actually, I had similar thoughts with
Obviously, under the hood Whereas if we just have
I think we can actually still include this, the above is perfectly reasonable. I was stopping short of
Also completely agree here. Actually, this was a concern of mine as well. Too much movement will break backward compatibility if not configurable. That will be a challenge for #61, but I do think it will be valuable if/when it is resolved.
Agree here as well. What about
Which aligns with everything else we've covered. Thoughts? If this looks good, I'll start heading in that direction. |
Yes, I think we're pretty aligned! There are still details that are a bit fuzzy to me, but my experience in Footnotes
|
Sounds good @brentyi! Yep I agree, things will definitely get firmer as we get further along 👍🏾 |
I have this pretty simple example with two nested optional settings objects. But when I run and only specify the second object
optimizer-settings:optimizer-settings
it doesn't work.I see a similar issue if I run
The text was updated successfully, but these errors were encountered: