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

Support PEP 727 Doc objects #279

Merged
merged 9 commits into from
Mar 28, 2025
Merged

Support PEP 727 Doc objects #279

merged 9 commits into from
Mar 28, 2025

Conversation

emcd
Copy link
Contributor

@emcd emcd commented Mar 23, 2025

Co-Authored-By: Anthropic claude-3-7-sonnet, xAI grok-3

Closes #167 .

Lints pass:

$ ruff check
All checks passed!
$ pyright .
0 errors, 0 warnings, 0 informations

Tests pass:

$ pytest -n auto
========================================================================================= test session starts =========================================================================================
platform linux -- Python 3.10.16, pytest-8.3.5, pluggy-1.5.0
rootdir: /home/me/src/THIRD_PARTY/tyro
configfile: pyproject.toml
plugins: xdist-3.6.1, typeguard-4.4.2, cov-6.0.0
16 workers [575 items]
............................................................................................................................................................................................... [ 33%]
............................................................................................................................................................................................... [ 66%]
............................................................................................................................................................................................... [ 99%]
.. 

@emcd
Copy link
Contributor Author

emcd commented Mar 23, 2025

All tests passed on Python 3.10... I guess we're still supporting unsupported versions of Python (3.7 and 3.8).

@emcd emcd force-pushed the pep727 branch 2 times, most recently from c51afed to 9cd1c9c Compare March 23, 2025 03:03
Copy link

codecov bot commented Mar 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.86%. Comparing base (eeaf4cd) to head (084d8f0).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #279      +/-   ##
==========================================
+ Coverage   99.82%   99.86%   +0.04%     
==========================================
  Files          33       33              
  Lines        2320     2282      -38     
==========================================
- Hits         2316     2279      -37     
+ Misses          4        3       -1     
Flag Coverage Δ
unittests 99.86% <100.00%> (+0.04%) ⬆️

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.

@emcd
Copy link
Contributor Author

emcd commented Mar 23, 2025

@brentyi : The typing_extensions package has not supported Python 3.7 since early September of 2023. The version of this package which is resolved for Python 3.7 is too old to have Doc.

Also, Python 3.7 end-of-life was June 27th, 2023. My unsolicited recommendation is that Tyro drop support for Python 3.7.

Would you be open to a PR to drop 3.7 (and maybe 3.8) from the test matrix and update the trove classifiers in pyproject.toml?

@brentyi
Copy link
Owner

brentyi commented Mar 23, 2025

Thanks @emcd!!

I went ahead and did the 3.7 changes in #280, since it enables some cleanup that's been on my mind for a while.

For the rest of the PR:

  • I'd prefer slightly to just support Doc by default, instead of expanding the API with a tyro.conf flag.
  • For where to make the change, it might be easier here:

    tyro/src/tyro/_fields.py

    Lines 90 to 113 in 9cd1c9c

    # Try to extract argconf overrides from type.
    _, argconfs = _resolver.unwrap_annotated(typ, _confstruct._ArgConfig)
    argconf = _confstruct._ArgConfig(
    None,
    None,
    help=None,
    help_behavior_hint=None,
    aliases=None,
    prefix_name=True,
    constructor_factory=None,
    )
    for overwrite_argconf in argconfs:
    # Apply any annotated argument configuration values.
    argconf = dataclasses.replace(
    argconf,
    **{
    field.name: getattr(overwrite_argconf, field.name)
    for field in dataclasses.fields(overwrite_argconf)
    if getattr(overwrite_argconf, field.name) is not None
    },
    )
    if argconf.help is not None:
    helptext = argconf.help
    • Similar to how we handle tyro.conf.arg(help=...), I think this would be simpler / cleaner than modifying every docstring check in the struct specs.
  • For the docs, could we move typing_extensions.Doc into its own section, and add a note discouraging its use? I could see this being removed eventually (cc Future of typing_extensions.Doc python/typing_extensions#443), so I'd prefer not to put it first in the list or accidentally endorse it for new projects.

@emcd
Copy link
Contributor Author

emcd commented Mar 23, 2025

Thanks @emcd!!

I went ahead and did the 3.7 changes in #280, since it enables some cleanup that's been on my mind for a while.

Excellent. Thank you!

(Technically, Python 3.8 has been EOL since last October and should be considered for removal too. But 3.7 was the immediate obstacle that I faced; glad to have that out of the way.)

For the rest of the PR:

  • I'd prefer slightly to just support Doc by default, instead of expanding the API with a tyro.conf flag.
  • For where to make the change, it might be easier here:

    tyro/src/tyro/_fields.py

    Lines 90 to 113 in 9cd1c9c

    # Try to extract argconf overrides from type.
    _, argconfs = _resolver.unwrap_annotated(typ, _confstruct._ArgConfig)
    argconf = _confstruct._ArgConfig(
    None,
    None,
    help=None,
    help_behavior_hint=None,
    aliases=None,
    prefix_name=True,
    constructor_factory=None,
    )
    for overwrite_argconf in argconfs:
    # Apply any annotated argument configuration values.
    argconf = dataclasses.replace(
    argconf,
    **{
    field.name: getattr(overwrite_argconf, field.name)
    for field in dataclasses.fields(overwrite_argconf)
    if getattr(overwrite_argconf, field.name) is not None
    },
    )
    if argconf.help is not None:
    helptext = argconf.help

Grok 3 also suggested that site and I initially tried intercepting there. However, the test that I added to test_conf.py did not tickle that code path, so I had to cast a wider net. I will give it second look later today.

  • Similar to how we handle tyro.conf.arg(help=...), I think this would be simpler / cleaner than modifying every docstring check in the struct specs.

I mostly followed your previous PR for optionally turning off the helptext comments for this part of the change. But, if we're not going to use a marker, then I can revert the changes to the struct specs. However, I would argue that passing the markers rather than a specialized feature bool is more future-proof. But, your call.

  • For the docs, could we move typing_extensions.Doc into its own section, and add a note discouraging its use? I could see this being removed eventually (cc Future of typing_extensions.Doc python/typing_extensions#443), so I'd prefer not to put it first in the list or accidentally endorse it for new projects.

Sure. I had put a warning on the new marker, but if we're dropping the marker, then I can make a separate section and transfer the warning there. However, I would like to present a unified view of the precedence of helptext sources: (1) specific metadata (help, description, etc...), (2) Doc annotations, (3) Sphinx-style attribute docstrings, (4) nearby comments. Do you have any particular thoughts on where/how to present this, @brentyi ?

Btw, even though Jelle was the sponsor of PEP 727 and had recommended that it be withdrawn last July, a significant amount feedback in support of the PEP did continue to trickle in after the fact and it is still in Draft status. He also said in the PEP discussion that typing_extensions would continue to support it indefinitely. While this is no guarantee that it won't go away, it is enough to embolden a fool, such as myself, to use it anyway since I think it is a good idea. That said, I do agree that there should be a strong admonition around it.

Co-Authored-By: Anthropic claude-3-7-sonnet, xAI grok-3
@brentyi
Copy link
Owner

brentyi commented Mar 23, 2025

I mostly followed your previous PR for optionally turning off the helptext comments for this part of the change. But, if we're not going to use a marker, then I can revert the changes to the struct specs. However, I would argue that passing the markers rather than a specialized feature bool is more future-proof. But, your call.

Ah yes, I agree that a feature bool isn't ideal. What I meant to suggest is to remove the ability to configure it altogether. It seems OK to me to leave this feature always on.

However, I would like to present a unified view of the precedence of helptext sources

This makes a lot of sense. As one possible structure, how about creating a section explicitly for precedence? For example:

  • Helptext from docstrings
    • Example for functions
    • Example for "struct" types (dataclass/namedtuple/pydantic/attrs/typeddict/etc)
  • Helptext from comments
    • Examples for "struct" types (dataclass/namedtuple/pydantic/attrs/typeddict/etc)
  • Helptext from annotations (works anywhere)
    • tyro.conf.arg()
    • PEP 727 Doc + warning
  • Precedence rules
    • tyro.conf.arg()
    • PEP 727 Doc
    • Docstrings
    • Comments

This way we can make the precedence rules explicit, while still putting the "recommended" ways to incorporate helptext first.

He also said in the PEP discussion that typing_extensions would continue to support it indefinitely.

This was a helpful read, thanks! I'll confess I find myself more on the anti-PEP 727 side 😛 (but still happy to support it)

@emcd
Copy link
Contributor Author

emcd commented Mar 23, 2025

I mostly followed your previous PR for optionally turning off the helptext comments for this part of the change. But, if we're not going to use a marker, then I can revert the changes to the struct specs. However, I would argue that passing the markers rather than a specialized feature bool is more future-proof. But, your call.

Ah yes, I agree that a feature bool isn't ideal. What I meant to suggest is to remove the ability to configure it altogether. It seems OK to me to leave this feature always on.

Done - the PEP 727 feature is now always on. I had to add some exception handling to mitigate pathological cases where type hint inspection would fail (some Pydantic objects with weak local references, some objects with incomplete type signatures on Python 3.8). Making this always-on allowed us to catch those cases before someone else caught them in the wild.

I left the markers plumbing from the structs intact, even though it is no longer necessary for this PR. It does simplify the code and maintenance for the HelptextFromCommentsOff marker and should make adding other structs or helptext-related markers easier in the future.

However, I would like to present a unified view of the precedence of helptext sources

This makes a lot of sense. As one possible structure, how about creating a section explicitly for precedence? For example:

  • Helptext from docstrings

    • Example for functions
    • Example for "struct" types (dataclass/namedtuple/pydantic/attrs/typeddict/etc)
  • Helptext from comments

    • Examples for "struct" types (dataclass/namedtuple/pydantic/attrs/typeddict/etc)
  • Helptext from annotations (works anywhere)

    • tyro.conf.arg()
    • PEP 727 Doc + warning
  • Precedence rules

    • tyro.conf.arg()
    • PEP 727 Doc
    • Docstrings
    • Comments

This way we can make the precedence rules explicit, while still putting the "recommended" ways to incorporate helptext first.

Done! Thanks for the feedback.

He also said in the PEP discussion that typing_extensions would continue to support it indefinitely.

This was a helpful read, thanks! I'll confess I find myself more on the anti-PEP 727 side 😛 (but still happy to support it)

Haha. Thank you for being so gracious, @brentyi . But, seriously, if you do not want this feature in your codebase, I won't press the issue.

Honestly, I've been thinking about writing a "simple" library that would support generating both documentation and CLIs purely from Doc objects and class/function docstrings (without parsing parameters and return types). "Simple" because I do not think that it would need to muck around with ASTs or parsing source lines at all. Mainly thinking about this, because, even if Tyro supports my CLI "needs", I still need to get PEP 727 support on the doc generation side to see fruition of the no-duplication, one-source-for-everything vision....

Anyway, I think I'm ready for another review when you have a chance.

(Note that we have a sticky Python 3.7 test because I opened this PR before it was removing from the matrix (and presumably the project's required tests list).)

(Also note that the Codecov API rate limits very heavily and that is where the Codecov failures have been coming from.)

@emcd emcd changed the title Implement optional processing of PEP 727 Doc objects. Implement processing of PEP 727 Doc objects. Mar 24, 2025
@brentyi
Copy link
Owner

brentyi commented Mar 24, 2025

Thanks for the quick update @emcd!

But, seriously, if you do not want this feature in your codebase, I won't press the issue.

Will certainly let you know if there's anything I don't want to support! (#254 is a contender 😆 but I haven't found time to properly think about it)

I just did some refactor to reuse tyro's existing metadata inspection logic. With this implementation I think the maintenance burden should be extremely low => I'm happy to support Doc for as long as it exists in typing_extensions.

If it looks okay to you I'll do a pass over the docs sometime this week and then merge 🙂

@emcd
Copy link
Contributor Author

emcd commented Mar 24, 2025

Thanks for the quick update @emcd!

Not as quick as your refactor, but I guess you know the code better. ;)

But, seriously, if you do not want this feature in your codebase, I won't press the issue.

Will certainly let you know if there's anything I don't want to support! (#254 is a contender 😆 but I haven't found time to properly think about it)

😆

I just did some refactor to reuse tyro's existing metadata inspection logic. With this implementation I think the maintenance burden should be extremely low => I'm happy to support Doc for as long as it exists in typing_extensions.

If it looks okay to you I'll do a pass over the docs sometime this week and then merge 🙂

Looks good, though I swear adding logic to the generic stuct make method was actually the first thing I tried when I set out to implement this and it didn't work for me. (Screenshot of Grok 3's original proposal attached as proof.) (I had an assertion to trigger when the code path was being hit and it was not being hit for some of the tests.) Will chalk this up to this being a foreign codebase to me and not understanding the flow completely (even with a second pair of eyes AIs)....
image

Thanks for the revisions.

@brentyi
Copy link
Owner

brentyi commented Mar 24, 2025

My guess is your original code would have worked if you swapped the order of the # If Pep727DocObjects is present and the # Include context markers code blocks. When returned by unwrap_annotated(), markers only contains markers that were added to the Annotated[...] of the current field's type typ. The "context markers" refers to markers from parents of the current field and/or tyro.cli's config= argument.

@brentyi brentyi changed the title Implement processing of PEP 727 Doc objects. Support PEP 727 Doc objects Mar 28, 2025
@brentyi brentyi merged commit 4933cac into brentyi:main Mar 28, 2025
13 of 14 checks passed
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.

Feature: Recognition of PEP 727 Doc objects in type annotations.
2 participants