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

chore: fix typing, formatting and tests #31

Merged
merged 9 commits into from
Oct 28, 2024
Merged

chore: fix typing, formatting and tests #31

merged 9 commits into from
Oct 28, 2024

Conversation

Askir
Copy link
Contributor

@Askir Askir commented Oct 17, 2024

The PR is based on the ruff changes earlier so overall it does:

  • Add ruff formatting and linting to CI (and makes it pass)
  • Adds pyright type checking to CI (and makes it pass)
  • Splits the large client.py file into separate files in a submodule
  • Makes the tests pass in CI by:
    • using vcr.py for replaying any requests that were sent to openai
    • running a docker-compose with the official timescale-ha image and testing against that

Note that the recording is extremely large because tiktoken downloads the whole encoding vocab through an http request.
I've added a filter for removing authorization tokens and cookies to vcr.py. But I do feel like this is a bit of a time bomb until someone accidentally commits a secret. If a filter doesn't catch a header and the author doesn't see it before pushing 🤷‍♂️

@Askir Askir changed the title chore: fix typing chore: fix typing and formatting Oct 18, 2024
@Askir Askir requested review from cevian and alejandrodnm October 18, 2024 00:28
@alejandrodnm
Copy link

Did you try pyright without strict?

@Askir
Copy link
Contributor Author

Askir commented Oct 18, 2024

@alejandrodnm without strict it looks a lot better:

4 errors, 0 warnings, 0 informations

But this allows untyped functions and Any returns which essentially allows circumventing the type checker altogether. Not great imo.

For reference this is how it looks with strict:

301 errors, 0 warnings, 0 informations

@Askir
Copy link
Contributor Author

Askir commented Oct 19, 2024

The main problem is that some libraries in the python ecosystem still don't have types (noteably asyncpg and pgvector).
For mypy I can set ignore_missing_imports = true, this then means the results are treated as Any.
This causes an error when I return an Any value from a function. I can however mark that return as #type: ignore and after that mypy "trusts" my annotation.

So for Asyncpg I (or rather Claude) typed the functions as returning an asyncpg.Record and simply ignored the error that Any is not a Record.

Pyright works differently, I can ignore the error in that location but it doesn't trust my annotation afterwards it instead build a union type Record | Unknown where Unknown is equivalent to mypy untyped Any. This then throws errors in the rest of the codebase because of course I handle it as if it was a Record everywhere.

I haven't really figured out how to change this behaviour, there doesn't seem to be away to say: Trust my annotation. The only recommendation that pyright has for untyped third party libraries is to write stub files yourself, which works but honestly is a bit too much effort right now. Asyncpg e.g. has a PR to add types since 4 years here.

I get that in pgai we use pyright and would need to use the same typechecker. It just seems that once you have some sort of untyped dependencies the pyright strict mode no longer really works. Kinda sucks 🤷‍♂️

@Askir
Copy link
Contributor Author

Askir commented Oct 25, 2024

I've fixed the typing issues by providing stubs for everything in use. Took a while but with Claudes help it was doable. So now we can use pyright in strict and dont have to change anything about the config for pgai. @alejandrodnm

@Askir Askir force-pushed the fix-typing branch 3 times, most recently from 8eb554b to 91bfb10 Compare October 25, 2024 01:17
@Askir Askir changed the title chore: fix typing and formatting chore: fix typing, formatting and tests Oct 28, 2024
@Askir Askir merged commit 7b177d0 into main Oct 28, 2024
4 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.

2 participants