Skip to content

Fix type parameter forwarding for inherited generics#328

Merged
brentyi merged 3 commits into
mainfrom
brent/fix_generic_inheritance_edge_case
Jul 29, 2025
Merged

Fix type parameter forwarding for inherited generics#328
brentyi merged 3 commits into
mainfrom
brent/fix_generic_inheritance_edge_case

Conversation

@brentyi

@brentyi brentyi commented Jul 28, 2025

Copy link
Copy Markdown
Owner

This PR fixes an edge case and adds test cases for inheritance structures that look like this:

class Base[T1]:
    field2: T1

class Subclass[T2](Base[T2]):
    field2: T2

where assignments for the T2 type parameter are applied to field2, but not correctly forwarded to field1.

cc #327

@codecov

codecov Bot commented Jul 28, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.87%. Comparing base (19c7af4) to head (57da453).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #328      +/-   ##
==========================================
- Coverage   99.87%   99.87%   -0.01%     
==========================================
  Files          35       35              
  Lines        2347     2346       -1     
==========================================
- Hits         2344     2343       -1     
  Misses          3        3              
Flag Coverage Δ
unittests 99.87% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@brentyi brentyi changed the title Fix edge case for inherited generics Fix type parameter forwarding edge case for inherited generics Jul 29, 2025
@brentyi brentyi changed the title Fix type parameter forwarding edge case for inherited generics Fix type parameter forwarding for inherited generics Jul 29, 2025
@brentyi
brentyi merged commit 647de8f into main Jul 29, 2025
16 checks passed
@brentyi
brentyi deleted the brent/fix_generic_inheritance_edge_case branch July 29, 2025 05:51
@mirceamironenco

Copy link
Copy Markdown
Contributor

Hi @brentyi,

This seems to have broken something for me, it's a functionality I implemented replicating your tyro.conf.configure:

import dataclasses
from typing import Annotated, Union

import tyro


@dataclasses.dataclass(kw_only=True, frozen=True)
class OptimizerConfig:
    lr: float = 1e-5
    weight_decay: float = 0.0


@dataclasses.dataclass(kw_only=True, frozen=True)
class AdamConfig(OptimizerConfig):
    betas: tuple[float, float] = (0.9, 0.999)
    eps: float = 1e-8
    amsgrad: bool = False
    fused: bool | None = None


@dataclasses.dataclass(kw_only=True, frozen=True)
class SGDConfig(OptimizerConfig):
    momentum: float = 0
    dampening: float = 0
    nesterov = False
    fused: bool | None = None


# works
# OptimizerConfig = Annotated[
#     OptimizerConfig,
#     tyro.conf.arg(constructor_factory=lambda: Union[AdamConfig, SGDConfig]),  # type: ignore
# ]

# crashes
OptimizerConfig.__tyro_markers__ = (  # type: ignore
    tyro.conf.arg(
        constructor_factory=lambda: Union[AdamConfig, SGDConfig]  # type: ignore
    ),
)


@dataclasses.dataclass
class Config:
    optimizer: OptimizerConfig = AdamConfig()


def main(config: Config) -> None:
    print(config)


if __name__ == "__main__":
    config = tyro.cli(Config)
    main(config)

On 0.9.27 this crashes for the case where I'm setting __tyro_markers__ directly, however this was working on 0.9.26. Do I have to switch away from this or is there some additional corner case missed by this PR?

brentyi added a commit that referenced this pull request Dec 1, 2025
* Fix edge case for inherited generics

* ruff

* More correct fix, test for deeper nesting
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

Successfully merging this pull request may close these issues.

2 participants