subcommand parsing with pydantic v1 models does not work.

[sander76]

When using pydantic v1 and creating a model with subcommands, the first model is always used.
See the test below. Using pydantic v1 this fails.

def test_pydantic_model_with_sub_commands_and_positionals__success():
    class SubCommandOne(BaseModel):
        name: str

    class SubCommandTwo(BaseModel):
        name: str

    class ModelWithSubcommand(BaseModel):
        sub_command: SubCommandOne | SubCommandTwo

    model = cli(
        ModelWithSubcommand,
        args=["sub-command:sub-command-two", "--sub-command.name", "myname"],
    )
    assert isinstance(model.sub_command, SubCommandTwo)
    assert model.sub_command.name == "myname"

[brentyi]

Hi @sander76!

tyro isn't doing anything special here, this seems like a result of Pydantic's internal union logic.

The error persists if we remove the tyro bits and instantiate things directly:

from pydantic import BaseModel


class SubCommandOne(BaseModel):
    name: str


class SubCommandTwo(BaseModel):
    name: str


class ModelWithSubcommand(BaseModel):
    sub_command: SubCommandOne | SubCommandTwo


# Prints `SubcommandOne`.
print(
    type(
        ModelWithSubcommand(
            sub_command=SubCommandTwo(name="myname"),
        ).sub_command
    )
)

Bot tyro and the vanilla pydantic usages are "fixed" once the fields of SubcommandOne and SubcommandTwo start differing.

Given this, do you think there's anything we could / should do here?

[sander76]

Hmm. Didn't expect your example to happen. But I think I do know why it does and I have seen it before, but not in your case where you explicitly define the sub_command.

But I guess the following is happening (and I think this is the case for tyro too)
Consider your above example and populate it using a dict:

model = ModelWithSubcommand(sub_command={"name":"myname"})

Pydantic has no way of knowing what the type of sub_command is. So it tries to instantiate all models going through the list of Unions. The first one that doesn't raise an issue, will be used. In this case both models have both the name as required property and the first model in the list of unions, SubCommandOne doesn't raise and is thus used.

Pydantic has the option of smart_union making Union check more thorough. This should work (but don't know how to exactly implement this in tyro):

from pydantic import BaseModel


class SubCommandOne(BaseModel):
    name: str


class SubCommandTwo(BaseModel):
    name: str


class ModelWithSubcommand(BaseModel):
    sub_command: SubCommandOne | SubCommandTwo

    class Config:
        smart_union = True


# Prints `SubcommandOne`.
print(
    ModelWithSubcommand(
        sub_command=SubCommandTwo(name="myname"),
    )
)

[brentyi]

Cool!

Adding the smart_union = True flag makes the assert in your test pass for me:

from pydantic import BaseModel
from tyro import cli

class SubCommandOne(BaseModel):
    name: str

class SubCommandTwo(BaseModel):
    name: str

class ModelWithSubcommand(BaseModel):
    sub_command: SubCommandOne | SubCommandTwo

    class Config:
        smart_union = True

model = cli(
    ModelWithSubcommand,
    args=["sub-command:sub-command-two", "--sub-command.name", "myname"],
)
assert isinstance(model.sub_command, SubCommandTwo)
assert model.sub_command.name == "myname"

[brentyi]

To comment a bit on what's happening internally: tyro is correctly instantiating a SubcommandTwo object in your original (failing) test, but when this is passed to ModelWithSubcommand older versions of pydantic seem to automatically throw away the SubcommandTwo instance and re-instantiate a SubcommandOne one. This is basically the same as what happens in the tyro-free example I included above.

I'm not sure that a workaround is even possible, but since this is resolved in newer versions of Pydantic my feeling is we probably don't need to worry about it on our end?

[sander76]

Ah. I see. I guess you're right.

But to prevent you from doing a lot of "needless" work, maybe even drop support for pydantic v1 altogether ?