nit: Fix/add some type annotations

[bradhilton]

Context

What is the purpose of this PR? Is it to

• add a new feature
• fix a bug
• update tests and/or documentation
• other (please add here)

Changelog

What are the changes made in this PR?

• Fix/add a few type annotations in torchtune.config._instantiate.py and touchtone.config._parse.py

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/1982

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 2e21126 with merge base ac4f88e ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[RdoubleA]

Thanks for the fix! Out of curiosity, is there a "correct" typing for *args and **kwargs? I thought they actually should be untyped. The updated return types look good to me, though.

should address comments

[bradhilton]

Thanks for the fix! Out of curiosity, is there a "correct" typing for *args and **kwargs? I thought they actually should be untyped. The updated return types look good to me, though.

You can type them, but there is a bit of nuance:

def foo(*args: Any, **kwargs: Any) -> None:
    args # inferred type here is tuple[Any, ...]
    kwargs # inferred type here is dict[str, Any]
    
def bar(*args: tuple[Any, ...], **kwargs: dict[str, Any]) -> None:
    args # inferred type here would be tuple[tuple[Any, ...], ...]
    kwargs # inferred type here would be dict[str, dict[str, Any]]

You can see that the type annotations are for the members of args and kwargs, not args and kwargs themselves. If you type them as tuples or dicts, then it will infer them to be nested tuples or nested dicts. If you try to call bar with something other than tuple args and dict kwargs then a type error will be presented and the user directed to pass tuples and dicts which is usually not the desired behavior.

You can leave *args and **kwargs untyped and it should infer tuple[Any, ...] and dict[str, Any] automatically. You can also narrow the type of args and kwargs like this for example:

def foobar(*strings: str, **ints: int) -> None:
    strings # inferred type is tuple[str, ...]
    ints # inferred type is dict[str, int]
    
foobar("hello", a=10, b=3) # okay
foobar(10, c="world") # not okay, args are expected to be strings and kwargs are expected to be integers

[RdoubleA]

@bradhilton just need to fix lint and I can stamp it

[bradhilton]

I think lint passed @RdoubleA

[bradhilton]

Test failing, need to revert some changes.

[felipemello1]

@bradhilton , thanks for the PR! Would you mind making it consistent in the other places that we have typing for args, kwargs?

https://github.com/search?q=repo%3Apytorch%2Ftorchtune%20*args%20(Tuple%5BAny%2C%20...%5D)&type=code

[felipemello1]

probably unintentional?

[bradhilton]

Yup! 🤦‍♂️

[felipemello1]

will merge after tests pass. Thanks!