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

Migrate to __future__.annotations #347

Merged
merged 1 commit into from
Nov 7, 2024
Merged

Migrate to __future__.annotations #347

merged 1 commit into from
Nov 7, 2024

Conversation

oerc0122
Copy link
Collaborator

@oerc0122 oerc0122 commented Nov 6, 2024

Following discussion quick migration to future annotations and | union syntax globally.

N.B. Does not update doc labels.

@oerc0122 oerc0122 self-assigned this Nov 6, 2024
@ElliottKasoar
Copy link
Member

I've been looking at this too, and somewhere the changes in janus_core/cli cause errors.

Also, if you haven't run ruff format after the fixes, the spacing will be wrong

@oerc0122
Copy link
Collaborator Author

oerc0122 commented Nov 6, 2024

I've been looking at this too, and somewhere the changes in janus_core/cli cause errors.

Also, if you haven't run ruff format after the fixes, the spacing will be wrong

Is that due to the fact that the Annotated is processed in typer?

@ElliottKasoar
Copy link
Member

ElliottKasoar commented Nov 6, 2024

There have been issues relating to that, which may not be completely solved (fastapi/typer#802), but this seems to be from TyperDict specifically, not Annotated, as I can "fix" cli/utils.py, cli/types.py, and cli/train.py without problems, but not cli/singlepoint.py, for example.

@oerc0122
Copy link
Collaborator Author

oerc0122 commented Nov 6, 2024

There have been issues relating to that, which may not be completely solved (fastapi/typer#802), but this seems to be from TyperDict specifically, not Annotated, as I can "fix" cli/utils.py, cli/types.py, and cli/train.py without problems, but not cli/singlepoint.py, for example.

We can add exceptions to the CLI files noting the issue, I guess.

@oerc0122
Copy link
Collaborator Author

oerc0122 commented Nov 6, 2024

We have a bit of a choice. Do we want file-level disabling of I002, FA100 (not importing future annotations)

#ruff: #noqa: I002, FA100

at the top of each CLI file or do we want a pyproject.toml level disable?

[tool.ruff.lint.per-file-ignores]
# Ignore __future__ annotations import in CLI folder.
"janus-core/cli/**.py" = ["I002", "FA100"]

@oerc0122 oerc0122 force-pushed the future-annotation branch 3 times, most recently from b478201 to 7bcce64 Compare November 6, 2024 15:59
@ElliottKasoar
Copy link
Member

ElliottKasoar commented Nov 6, 2024

So actually I think typer itself may not actually to blame.

It seems to be specifically caused by @use_config (from typer-config, rather than typer), in combination with the custom types and annotations, because it uses inspect.signature:

    def decorator(cmd: TyperCommand) -> TyperCommand:
        # NOTE: modifying a function's __signature__ is dangerous
        # in the sense that it only affects inspect.signature().
        # It does not affect the actual function implementation.
        # So, a caller can be confused how to pass parameters to
        # the function with modified signature.
        sig = signature(cmd)

        config_param = Parameter(
            param_name,
            kind=Parameter.KEYWORD_ONLY,
            annotation=str,
            default=Option("", callback=callback, is_eager=True, help=param_help),
        )

        new_sig = sig.replace(parameters=[*sig.parameters.values(), config_param])

        @wraps(cmd)
        def wrapped(*args, **kwargs):  # noqa: ANN202,ANN002,ANN003
            # NOTE: need to delete the config parameter
            # to match the wrapped command's signature.
            if param_name in kwargs:
                del kwargs[param_name]

            return cmd(*args, **kwargs)

        wrapped.__signature__ = new_sig  # type: ignore

        return wrapped

    return decorator

which I think means the custom info isn't copied properly.

signature seems to generally have problems, with solutions e.g. eval_str only introduced for 3.10+.

@ElliottKasoar
Copy link
Member

ElliottKasoar commented Nov 7, 2024

We have a bit of a choice. Do we want file-level disabling of I002, FA100 (not importing future annotations)

#ruff: #noqa: I002, FA100

at the top of each CLI file or do we want a pyproject.toml level disable?

[tool.ruff.lint.per-file-ignores]
# Ignore __future__ annotations import in CLI folder.
"janus-core/cli/**.py" = ["I002", "FA100"]

I don't mind too much, but being explicit per file is maybe a bit clearer, so I'm happy with the current form.

Only thing left is I think the explanations should be updated.

I should probably open an issue in typer-config, which we can reference? (I was going to do a PR too to set eval_str, before discovering that the solution doesn't work for Python 3.9)

@ElliottKasoar ElliottKasoar merged commit ae46a4e into main Nov 7, 2024
16 checks passed
@ElliottKasoar ElliottKasoar deleted the future-annotation branch November 7, 2024 14:35
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