search

brentyi / tyro

CLI interfaces & config objects, from types
https://brentyi.github.io/tyro
MIT License
467 stars 23 forks source link

Make _fields.py assertion much more helpful #131

Closed pwais closed 5 months ago

pwais commented 6 months ago

Tracebacks like these are unhelpful, especially in nerfstudio where there are literally eleventeen brazilian fields that could be broken:

ns-train         --help
Traceback (most recent call last):
  File "/usr/local/bin/ns-train", line 8, in <module>
    sys.exit(entrypoint())
  File "/opt/nerfstudio/scripts/train.py", line 263, in entrypoint
    tyro.cli(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_cli.py", line 187, in cli
    output = _cli_impl(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_cli.py", line 374, in _cli_impl
    parser_spec = _parsers.ParserSpecification.from_callable_or_type(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 106, in from_callable_or_type
    field_out = handle_field(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 279, in handle_field
    subparsers_attempt = SubparsersSpecification.from_field(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 443, in from_field
    subparser = ParserSpecification.from_callable_or_type(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 106, in from_callable_or_type
    field_out = handle_field(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 304, in handle_field
    return ParserSpecification.from_callable_or_type(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 106, in from_callable_or_type
    field_out = handle_field(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 304, in handle_field
    return ParserSpecification.from_callable_or_type(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 106, in from_callable_or_type
    field_out = handle_field(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 296, in handle_field
    if _fields.is_nested_type(field.typ, field.default):
  File "/usr/local/lib/python3.10/dist-packages/tyro/_unsafe_cache.py", line 33, in wrapped_f
    out = f(*args, **kwargs)
  File "/usr/local/lib/python3.10/dist-packages/tyro/_fields.py", line 188, in is_nested_type
    _try_field_list_from_callable(typ, default_instance),
  File "/usr/local/lib/python3.10/dist-packages/tyro/_fields.py", line 298, in _try_field_list_from_callable
    return field_list_from_class(cls, default_instance)
  File "/usr/local/lib/python3.10/dist-packages/tyro/_fields.py", line 537, in _field_list_from_attrs
    assert attr_field.type is not None
AssertionError

With this change, now you can figure out wat teh brizzoke really much faster!

ns-train         --help
Traceback (most recent call last):
  File "/usr/local/bin/ns-train", line 8, in <module>
    sys.exit(entrypoint())
  File "/opt/nerfstudio/scripts/train.py", line 263, in entrypoint
    tyro.cli(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_cli.py", line 187, in cli
    output = _cli_impl(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_cli.py", line 374, in _cli_impl
    parser_spec = _parsers.ParserSpecification.from_callable_or_type(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 106, in from_callable_or_type
    field_out = handle_field(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 279, in handle_field
    subparsers_attempt = SubparsersSpecification.from_field(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 443, in from_field
    subparser = ParserSpecification.from_callable_or_type(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 106, in from_callable_or_type
    field_out = handle_field(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 304, in handle_field
    return ParserSpecification.from_callable_or_type(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 106, in from_callable_or_type
    field_out = handle_field(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 304, in handle_field
    return ParserSpecification.from_callable_or_type(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 106, in from_callable_or_type
    field_out = handle_field(
  File "/usr/local/lib/python3.10/dist-packages/tyro/_parsers.py", line 296, in handle_field
    if _fields.is_nested_type(field.typ, field.default):
  File "/usr/local/lib/python3.10/dist-packages/tyro/_unsafe_cache.py", line 33, in wrapped_f
    out = f(*args, **kwargs)
  File "/usr/local/lib/python3.10/dist-packages/tyro/_fields.py", line 188, in is_nested_type
    _try_field_list_from_callable(typ, default_instance),
  File "/usr/local/lib/python3.10/dist-packages/tyro/_fields.py", line 298, in _try_field_list_from_callable
    return field_list_from_class(cls, default_instance)
  File "/usr/local/lib/python3.10/dist-packages/tyro/_fields.py", line 537, in _field_list_from_attrs
    assert attr_field.type is not None, attr_field
AssertionError: Attribute(name='save_debug_assets', default=False, validator=None, repr=True, eq=True, eq_key=None, order=True, order_key=None, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False, inherited=False, on_setattr=None, alias='save_debug_assets')

➡️ Always leave breadcrumbs / messages in asserts. In python the user will almost never see the local var values unless they're using e.g. loguru / better-exceptions / pytest.

IMO tyro is sufficiently complexicationated that all the asserts should have useful messages. So why did I get this assert in the first place? I have no idea tyro is too complexicationated! EI_TOO_MUCH_MAGIC 🪄🪄

pwais commented 6 months ago

@brentyi

brentyi commented 6 months ago

Thanks for the PR! Just to check: how did you end up fixing this? (what was causing the assert to fail?)

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.58%. Comparing base (28082f8) to head (8d03eef).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #131 +/- ## ======================================= Coverage 99.58% 99.58% ======================================= Files 24 24 Lines 1951 1951 ======================================= Hits 1943 1943 Misses 8 8 ``` | [Flag](https://app.codecov.io/gh/brentyi/tyro/pull/131/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Brent+Yi) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/brentyi/tyro/pull/131/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Brent+Yi) | `99.58% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Brent+Yi#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

pwais commented 6 months ago

i found out tryo appears to not like attrs any more. tyro worked for me for a long time with attrs but i switched to dataclass and magically the error goes away. i even downgraded tyro but got this traceback so i have no idea why tyro broke when i upgraded nerfstudio 🤷 🤷 🤷 🤷 to be frank, while i appreciate the difficulty of what tyro does, i am not a fan of it being part of nerfstudio, which already has tons of API instability and crazy bugs on its own. i mean like imagine if yangqing jia decided to include a home-rolled protobuff instead of regular protobuf in caffe.

brentyi commented 6 months ago

If you have a runnable example for attrs failing that would be appreciated, the attrs tests still pass but maybe some edge case is being missed?

to be frank, while i appreciate the difficulty of what tyro does, i am not a fan of it being part of nerfstudio, which already has tons of API instability and crazy bugs on its own. i mean like imagine if yangqing jia decided to include a home-rolled protobuff instead of regular protobuf in caffe.

Thanks for your feedback, I still like tyro a lot but do recognize there are limitations 😛 Particular room for improvement in error messages + API clarity in terms of what's actually supported vs not supported.