jsonargparse icon indicating copy to clipboard operation
jsonargparse copied to clipboard

`from __future__ import annotations` support

Open carmocca opened this issue 2 years ago • 3 comments

These 2 should be equal on Python>=3.7

from typing import Optional

from jsonargparse import ArgumentParser


class Foo:
    def __init__(self, x: Optional[str] = None) -> None:
        self.x = x

parser = ArgumentParser()
parser.add_class_arguments(Foo)
args = parser.parse_args()
print(args)  # Namespace(x=None)
from __future__ import annotations

from jsonargparse import ArgumentParser


class Foo:
    def __init__(self, x: str | None = None) -> None:
        self.x = x

parser = ArgumentParser()
parser.add_class_arguments(Foo)
args = parser.parse_args()
print(args)  # Namespace()

Additional context

We want to adopt this in Lightning: https://github.com/PyTorchLightning/pytorch-lightning/issues/11205

carmocca avatar Feb 01 '22 16:02 carmocca

Some support for future annotations in python 3.7-3.9 could be added. However, your main motivation for it is the use of the new syntax for unions using a vertical bar. I don't see support for this being added any time soon. With PEP 563 by postponing the evaluation of annotations, the import and use of the module does not fail because the annotations become strings. But jsonargparse would need to evaluate them at runtime. And that syntax is not a valid expression in python 3.7-3.9, so there is no native way to evaluate it. The problem can be observed with the following:

from __future__ import annotations
from typing import get_type_hints

def my_func(param: int | str):
    ...

print(get_type_hints(my_func))

In python 3.7-3.9 this fails in the last line with TypeError: unsupported operand type(s) for |: 'type' and 'type'. Implementing a parser for the new syntax in jsonargparse would be out of the question. There is a new project that is trying to implement this parsing https://github.com/PrettyWood/future-typing. But it is still too immature to consider adding it as a dependency or even to assume that it will work correctly on most cases.

On the other hand, making PEP 563 the default was initially planned for python 3.10. But it was postponed because there is a bit of a controversy around this. A good summary can be found at https://lwn.net/Articles/858576/. Better to wait until it is more definite how things play out and there are more reliable tools for runtime evaluation. Particularly in the case of jsonargparse which does not yet have a community that would help in maintaining it.

mauvilsa avatar Feb 02 '22 07:02 mauvilsa

The Lightning issue linked above suggest adopting 3 PEPs together: PEP 585, PEP 604, and PEP 563.

Out of those, 585 and 563 are the most interesting. 585 to avoid users having to add the imports when their IDE auto-fills a method implementation and 563 to reduce circular dependencies.

If 604 and 563 cannot be supported yet here, I guess it means we cannot convert the code in Lightning yet.

Is there a problem with 585 though?

carmocca avatar Feb 02 '22 20:02 carmocca

Is there a problem with 585 though?

It has the same problem, no native way to evaluate the type hint.

from __future__ import annotations
from typing import get_type_hints

def my_func(param: list[int]):
    ...

print(get_type_hints(my_func))

This produces TypeError: 'type' object is not subscriptable.

mauvilsa avatar Feb 04 '22 07:02 mauvilsa

Some time ago I added backporting of PEPs 585 and 604 types in python 3.7-3.9, though this was only used to get types from stub (*.pyi) files. Even though it has been out and used for a while, bugs in the backport might no have been discovered, because failures are silent and the stub types simply not resolved.

PEP 563 is now implemented (see pull request #294) including the backports for python 3.7-3.9. All the unit tests are now run using future annotations. Nonetheless, there might be bugs which would be good to discover before including this in a release. @carmocca how about trying the pyupgrade of lightning in a branch and temporarily changing the requirement to jsonargparse[signatures] @ https://github.com/omni-us/jsonargparse/zipball/future-annotations to test things out?

mauvilsa avatar Jun 06 '23 05:06 mauvilsa

Sure. You can open a PR on Lightning with the changes to let our CI run.

carmocca avatar Jun 06 '23 14:06 carmocca