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

Lots of type issues reported by pyright. Not sure if its me or the library? #112

Closed
LTSCommerce opened this issue Jul 26, 2024 · 3 comments
Assignees
Labels
bug Something isn't working confirmed This issue is confirmed work in progress Users assigned to this issue/PR are working on it

Comments

@LTSCommerce
Copy link

Running pyright on a project using injectable and I get lots of errors such as

error: Arguments missing for parameters "_log", "_local", "_mock", "_openai", "_constants" (reportCallIssue)
error: Call expression not allowed in type expression (reportInvalidTypeForm)

for code that looks like

from typing_extensions import Annotated
from services.chat import Chat
from llama_index.core.llms.llm import LLM
import typer
from injectable import autowired, Autowired
from services.constants import Constants
from services.logger import Logger
import services.llm_factory as factory

class ChatApp:
    @autowired
    def __init__(
        self,
        _log: Autowired(Logger),
        _local: Autowired(factory.LlamafileFactory),
        _mock: Autowired(factory.MockLLMFactory),
        _openai: Autowired(factory.OpenAIFactory),
        _constants: Autowired(Constants),
    ):
        self._log = _log
        self._localFactory = _local
        self._mockFactory = _mock
        self._openaiFactory = _openai
        self._constants = _constants

Now I'm rereading the docs, I don't see any notes about compatability with type checking tools, so I'm wondering if this is a known limitation and its not really possible to use this library along with pyright?

@LTSCommerce LTSCommerce added the bug Something isn't working label Jul 26, 2024
@roo-oliv
Copy link
Owner

roo-oliv commented Jul 30, 2024

Hi @LTSCommerce! Thanks for submitting your issue!

I'm not familiar with pyright but if I understand correctly you're getting two errors:

  1. error: Arguments missing for parameters "_log", "_local", "_mock", "_openai", "_constants" (reportCallIssue)

This seems like a static (?) analysis pointing that you're invoking the function with missing arguments. This seems more like a pyright convention or a bug because the function signature is changed by the @autowired decorator and, apparently, this is being ignored by pyright.

By design injectable doesn't allow autowired parameters to have default values because it would overwrite them, impairing readability and causing confusion.

If pyright allows for skipping this specific check, I'd advise this solution. If this is possible, let me know and we can add observations to the Caveats section in the docs. If this is not possible or you believe this is not an acceptable solution I'd need some time to investigate if we should propose changes to pyright or consider changes to injectable so we offer a good workaround.

  1. error: Call expression not allowed in type expression (reportInvalidTypeForm)

This issue seems similar to mypy behavior cited in this issue: python/mypy#10277 and this other issue: microsoft/pylance-release#5457 (comment)

There are two solutions to this. The simpler one is to declare those outside of the type annotation, like this:

AutowiredBar = Autowired(Bar)
...
@autowired
def foo(bar: AutowiredBar): ...

I consider though that this is less readable and the second options seems like a better alternative which is to use typing.Annotated like this:

@autowired
def foo(bar: Annotated[Bar, Autowired]: ...

When using Annotated you can use call expressions in the typehint metadata:

@autowired
def foo(bar: Annotated[Bar, Autowired(lazy=true)]: ...

Said that, there is the possibility to change injectable to allow the use of Autowired as a Generic type, like this:

@autowired
def foo(bar: Autowired[Bar]): ...

But I'd need to think carefully on how to implement arguments for injection, maybe something like this but I have mixed feelings:

# this:
@autowired
def foo(bar: Autowired(List[Bar], lazy=true, qualifier="BarQualifier"): ...

# would be equivalent to:
@autowired
def foo(bar: Autowired[List[Bar], Lazy, Qualifier["BarQualifier"]]: ...

# or maybe:
@autowired
def foo(bar: Autowired[List[Lazy[Qualifier[Bar, "BarQualifier"]]]]: ...

@LTSCommerce
Copy link
Author

Thanks for the detailed reply, much appreciated!

Yes pyright is static analysis for type checking, it's an alternative to mypy that is apparently a lot faster.

I'm new to Python but do really like to have DI as it really enables highly modular and testable code. Even more though, I really like to have strict type checking as it just catches so many issues, and type hints make code a lot more readable and easier to grok.

If the autowire rewrites the code, maybe the answer is to somehow trigger that rewrite before pyright does it's thing. I've no idea how that might work though.

For now I've abandoned DI, but I may well come back to it.

FYI my background is mainly in PHP and if I could find the equivalent of https://symfony.com/doc/current/components/dependency_injection.html in python I'd be very happy

@roo-oliv
Copy link
Owner

If the autowire rewrites the code, maybe the answer is to somehow trigger that rewrite before pyright does it's thing. I've no idea how that might work though.

Actually in Python since there is no compilation or build step there is no actual rewrite of the code but rather a change of the function signature at evaluation time. Meaning you don't need to worry about when pyright is run.

I found a bug indeed in the @autowired decorator that causes this issue with pyright, mypy, and possibly others. The @autowired decorator is mistakenly declaring that the function signature stays the same and this is not true. I'll release a fix soon!

Thanks for contributing back to this project!

@roo-oliv roo-oliv self-assigned this Jul 31, 2024
@roo-oliv roo-oliv added confirmed This issue is confirmed work in progress Users assigned to this issue/PR are working on it labels Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working confirmed This issue is confirmed work in progress Users assigned to this issue/PR are working on it
Projects
None yet
Development

No branches or pull requests

2 participants