fix: mypy type errors in the examples and clean up #1132
Conversation
There was a problem hiding this comment.
Why do we rename these files? Please revert these changes.
It's unnecessary and will break existing links to these directoreis.
There was a problem hiding this comment.
There is also a Python PIP dependency called face_recognition, let me try reverting and testing
There was a problem hiding this comment.
They're top-level directories for applications. We don't need __init__.py for them.
There was a problem hiding this comment.
examples\azure_blob_embedding\main.py: error: Duplicate module named "main" (also at "examples\amazon_s3_embedding\main.py")
examples\azure_blob_embedding\main.py: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules for more info
examples\azure_blob_embedding\main.py: note: Common resolutions include: a) using `--exclude` to avoid checking one of them, b) adding `__init__.py` somewhere, c) using `--explicit-package-bases` or adjusting MYPYPATH
You get this error otherwise.
So we will have to run the examples mypy check separately on the CI since --explicit-package-bases is a global flag and not a module level flag. therefore cannot be set in pyproject.toml
There was a problem hiding this comment.
Yes, I think running each example as a separate project is exactly what we want.
It's also aligned with how users use cocoindex (they have their own separate project). The goal of these are setting examples for users so they can start to use cocoindex easily.
pyproject.toml
Outdated
| "qdrant_client", | ||
| "psycopg_pool", | ||
| "pgvector.psycopg", | ||
| "psycopg.rows", | ||
| "marker.*", | ||
| "pypdf", | ||
| "pdf2image", | ||
| "markitdown", | ||
| "openai", | ||
| "fastapi.*", | ||
| "uvicorn", | ||
| "face_recognition", |
There was a problem hiding this comment.
Please don't add them here.
They're only used by examples, not the cocoindex library. Examples depends on the library, not the other way, so we shouldn't introduce dependency for the library because of we use them in examples.
There was a problem hiding this comment.
It's not a python PIP dependency addition, I am just excluding these modules from producing an mypy missing imports error.
You can refer to the description of the PR for the [import-not-found] errors
| # --- Response Models --- | ||
|
|
||
|
|
||
| class SearchResult(BaseModel): |
There was a problem hiding this comment.
Consider use a dataclass here instead, which is available in Python standard library. In this way we keep these examples simple. We want to keep them as simple as possible; users don't need to install and understand another library unless it's really needed for the example.
| @@ -1,5 +1,5 @@ | |||
| from dotenv import load_dotenv | |||
| from psycopg_pool import ConnectionPool | |||
| from psycopg_pool import ConnectionPool # type: ignore[import-not-found] | |||
There was a problem hiding this comment.
Why do we need to add this?
It's already included in amazon_s3_embedding/pyproject.toml. As long as we run pip install . under the specific example directory, we shouldn't get "import not found", right?
There was a problem hiding this comment.
Right, I was under the impression that we would be running mypy with the venv of the main project. Will install dependencies per example and retest
resolves #1091
cc: @badmonster0
All the mypy errors for reference:
pyo3 didn't like my machine's python installation so did all my testing on Github Action runner 😄