mypy: analyze types in all possible libraries

Preparation for #646

* Remove `ignore_missing_imports = true` from mypy configuration.
* Run mypy in the same environment instead of separate to check types
  in dependencies like fastapi.
* Move mypy dependencies from pre-commit configuration to `setup.cfg`.
* Update mypy dependencies there.
* Move `rq` from `environment.yml` to `setup.cfg`: conda-forge version:
  1.9.0, pypi version : 1.15.1 (two years difference; types were added).
* Add libraries with missing types to ignore list in mypy
  configuration.
* Add pydantic mypy plugin.
* Allow running mypy without explicit paths.
* Update GitHub Actions.
* Temporarily add ignore `annotation-unchecked` to make mypy pass.
* Fix new mypy issues:
  * Use https://github.com/hauntsaninja/no_implicit_optional to make
    `Optional` explicit.
  * If there is no default, the first `pydantic.Field` argument should
    be omitted (`None` means that the default argument is `None`).
  * Refactor `_run_migrations`. There were two different paths: one for
    normal execution and another one for testing. Simplify arguments and
    the function code, and introduce a new mock `run_migrations`.
  * To preserve compatibility, introduce `ChannelWithOptionalName` for
    the `/channels` patch method. Note that the solution is a bit dirty
    (I had to use `type: ignore[assignment]`) to minimize the number of
    models and the diff.
  * Trivial errors.

Author: rominf

.github/workflows/lint.yml

@@ -17,6 +17,9 @@ jobs:
       - name: Add micromamba to GITHUB_PATH
         run: echo "${HOME}/micromamba-bin" >> "$GITHUB_PATH"
       - run: ln -s "${CONDA_PREFIX}" .venv # Necessary for pyright.
+      - run: pip install -e .[mypy]
+      - name: Add mypy to GITHUB_PATH
+        run: echo "${GITHUB_WORKSPACE}/.venv/bin" >> "$GITHUB_PATH"
       - uses: pre-commit/action@v3.0.0
         with:
           extra_args: --all-files --show-diff-on-failure

.pre-commit-config.yaml

@@ -19,19 +19,7 @@ repos:
     hooks:
       - id: mypy
         files: ^quetz/
-        additional_dependencies:
-          - sqlalchemy-stubs
-          - types-click
-          - types-Jinja2
-          - types-mock
-          - types-orjson
-          - types-pkg-resources
-          - types-redis
-          - types-requests
-          - types-six
-          - types-toml
-          - types-ujson
-          - types-aiofiles
+        language: system
         args: [--show-error-codes]
   - repo: https://github.com/Quantco/pre-commit-mirrors-prettier
     rev: 2.7.1

environment.yml

@@ -44,7 +44,6 @@ dependencies:
   - pre-commit
   - pytest
   - pytest-mock
-  - rq
   - libcflib
   - mamba
   - conda-content-trust

pyproject.toml

@@ -58,14 +58,33 @@ venv = ".venv"
 venvPath= "."
 
 [tool.mypy]
-ignore_missing_imports = true
+packages = [
+  "quetz"
+]
 plugins = [
+  "pydantic.mypy",
   "sqlmypy"
 ]
 disable_error_code = [
+  "annotation-unchecked",
   "misc"
 ]
 
+[[tool.mypy.overrides]]
+module = [
+  "adlfs",
+  "authlib",
+  "authlib.*",
+  "fsspec",
+  "gcsfs",
+  "pamela",
+  "sqlalchemy_utils",
+  "sqlalchemy_utils.*",
+  "s3fs",
+  "xattr"
+]
+ignore_missing_imports = true
+
 [tool.coverage.run]
 omit = [
   "quetz/tests/*",

quetz/cli.py

@@ -87,20 +87,17 @@ def _alembic_config(db_url: str) -> AlembicConfig:
 
 
 def _run_migrations(
-    db_url: Optional[str] = None,
-    alembic_config: Optional[AlembicConfig] = None,
+    db_url: str,
     branch_name: str = "heads",
 ) -> None:
-    if db_url:
-        if db_url.startswith("postgre"):
-            db_engine = "PostgreSQL"
-        elif db_url.startswith("sqlite"):
-            db_engine = "SQLite"
-        else:
-            db_engine = db_url.split("/")[0]
-        logger.info('Running DB migrations on %s', db_engine)
-        if not alembic_config:
-            alembic_config = _alembic_config(db_url)
+    if db_url.startswith("postgre"):
+        db_engine = "PostgreSQL"
+    elif db_url.startswith("sqlite"):
+        db_engine = "SQLite"
+    else:
+        db_engine = db_url.split("/")[0]
+    logger.info('Running DB migrations on %s', db_engine)
+    alembic_config = _alembic_config(db_url)
     command.upgrade(alembic_config, branch_name)
 
 
@@ -135,6 +132,7 @@ def _make_migrations(
     logger.info('Making DB migrations on %r for %r', db_url, plugin_name)
     if not alembic_config and db_url:
         alembic_config = _alembic_config(db_url)
+    assert alembic_config is not None
 
     # find path
     if plugin_name == "quetz":
@@ -594,7 +592,7 @@ def start(
         uvicorn.run(
             "quetz.main:app",
             reload=reload,
-            reload_dirs=(quetz_src,),
+            reload_dirs=[quetz_src],
             port=port,
             proxy_headers=proxy_headers,
             host=host,

quetz/config.py

@@ -230,7 +230,7 @@ class Config:
 
     _instances: Dict[Optional[str], "Config"] = {}
 
-    def __new__(cls, deployment_config: str = None):
+    def __new__(cls, deployment_config: Optional[str] = None):
         if not deployment_config and None in cls._instances:
             return cls._instances[None]
 
@@ -254,7 +254,7 @@ def __getattr__(self, name: str) -> Any:
         super().__getattr__(self, name)
 
     @classmethod
-    def find_file(cls, deployment_config: str = None):
+    def find_file(cls, deployment_config: Optional[str] = None):
         config_file_env = os.getenv(f"{_env_prefix}{_env_config_file}")
         deployment_config_files = []
         for f in (deployment_config, config_file_env):

quetz/dao.py

@@ -926,8 +926,8 @@ def create_version(
     def get_package_versions(
         self,
         package,
-        time_created_ge: datetime = None,
-        version_match_str: str = None,
+        time_created_ge: Optional[datetime] = None,
+        version_match_str: Optional[str] = None,
         skip: int = 0,
         limit: int = -1,
     ):

quetz/hooks.py

@@ -11,7 +11,7 @@
 
 
 @hookspec
-def register_router() -> 'fastapi.APIRouter':
+def register_router() -> 'fastapi.APIRouter':  # type: ignore[empty-body]
     """add extra endpoints to the url tree.
 
     It should return an :py:class:`fastapi.APIRouter` with new endpoints definitions.

quetz/jobs/rest_models.py

@@ -83,7 +83,7 @@ def parse_job_name(v):
 class JobBase(BaseModel):
     """New job spec"""
 
-    manifest: str = Field(None, title='Name of the function')
+    manifest: str = Field(title='Name of the function')
 
     start_at: Optional[datetime] = Field(
         None, title="date and time the job should start, if None it starts immediately"
@@ -110,35 +110,35 @@ def validate_job_name(cls, function_name):
 class JobCreate(JobBase):
     """Create job spec"""
 
-    items_spec: str = Field(..., title='Item selector spec')
+    items_spec: str = Field(title='Item selector spec')
 
 
 class JobUpdateModel(BaseModel):
     """Modify job spec items (status and items_spec)"""
 
-    items_spec: str = Field(None, title='Item selector spec')
-    status: JobStatus = Field(None, title='Change status')
+    items_spec: Optional[str] = Field(None, title='Item selector spec')
+    status: JobStatus = Field(title='Change status')
     force: bool = Field(False, title="force re-running job on all matching packages")
 
 
 class Job(JobBase):
-    id: int = Field(None, title='Unique id for job')
-    owner_id: uuid.UUID = Field(None, title='User id of the owner')
+    id: int = Field(title='Unique id for job')
+    owner_id: uuid.UUID = Field(title='User id of the owner')
 
-    created: datetime = Field(None, title='Created at')
+    created: datetime = Field(title='Created at')
 
-    status: JobStatus = Field(None, title='Status of the job (running, paused, ...)')
+    status: JobStatus = Field(title='Status of the job (running, paused, ...)')
 
     items_spec: Optional[str] = Field(None, title='Item selector spec')
     model_config = ConfigDict(from_attributes=True)
 
 
 class Task(BaseModel):
-    id: int = Field(None, title='Unique id for task')
-    job_id: int = Field(None, title='ID of the parent job')
-    package_version: dict = Field(None, title='Package version')
-    created: datetime = Field(None, title='Created at')
-    status: TaskStatus = Field(None, title='Status of the task (running, paused, ...)')
+    id: int = Field(title='Unique id for task')
+    job_id: int = Field(title='ID of the parent job')
+    package_version: dict = Field(title='Package version')
+    created: datetime = Field(title='Created at')
+    status: TaskStatus = Field(title='Status of the task (running, paused, ...)')
 
     @field_validator("package_version", mode="before")
     @classmethod

quetz/main.py

@@ -326,7 +326,7 @@ def get_users_handler(dao, q, auth, skip, limit):
 @api_router.get("/users", response_model=List[rest_models.User], tags=["users"])
 def get_users(
     dao: Dao = Depends(get_dao),
-    q: str = None,
+    q: Optional[str] = None,
     auth: authorization.Rules = Depends(get_rules),
 ):
     return get_users_handler(dao, q, auth, 0, -1)
@@ -341,7 +341,7 @@ def get_paginated_users(
     dao: Dao = Depends(get_dao),
     skip: int = 0,
     limit: int = PAGINATION_LIMIT,
-    q: str = None,
+    q: Optional[str] = None,
     auth: authorization.Rules = Depends(get_rules),
 ):
     return get_users_handler(dao, q, auth, skip, limit)
@@ -521,7 +521,7 @@ def set_user_role(
 def get_channels(
     public: bool = True,
     dao: Dao = Depends(get_dao),
-    q: str = None,
+    q: Optional[str] = None,
     auth: authorization.Rules = Depends(get_rules),
 ):
     """List all channels"""
@@ -540,7 +540,7 @@ def get_paginated_channels(
     skip: int = 0,
     limit: int = PAGINATION_LIMIT,
     public: bool = True,
-    q: str = None,
+    q: Optional[str] = None,
     auth: authorization.Rules = Depends(get_rules),
 ):
     """List all channels, as a paginated response"""
@@ -780,7 +780,7 @@ def post_channel(
     response_model=rest_models.ChannelBase,
 )
 def patch_channel(
-    channel_data: rest_models.Channel,
+    channel_data: rest_models.ChannelWithOptionalName,
     dao: Dao = Depends(get_dao),
     auth: authorization.Rules = Depends(get_rules),
     channel: db_models.Channel = Depends(get_channel_or_fail),
@@ -1054,8 +1054,8 @@ def post_package_member(
 def get_package_versions(
     package: db_models.Package = Depends(get_package_or_fail),
     dao: Dao = Depends(get_dao),
-    time_created__ge: datetime.datetime = None,
-    version_match_str: str = None,
+    time_created__ge: Optional[datetime.datetime] = None,
+    version_match_str: Optional[str] = None,
 ):
     version_profile_list = dao.get_package_versions(
         package, time_created__ge, version_match_str
@@ -1079,8 +1079,8 @@ def get_paginated_package_versions(
     dao: Dao = Depends(get_dao),
     skip: int = 0,
     limit: int = PAGINATION_LIMIT,
-    time_created__ge: datetime.datetime = None,
-    version_match_str: str = None,
+    time_created__ge: Optional[datetime.datetime] = None,
+    version_match_str: Optional[str] = None,
 ):
     version_profile_list = dao.get_package_versions(
         package, time_created__ge, version_match_str, skip, limit

quetz/metrics/view.py

@@ -1,5 +1,6 @@
 import os
 
+from fastapi import FastAPI
 from prometheus_client import (
     CONTENT_TYPE_LATEST,
     REGISTRY,
@@ -9,7 +10,6 @@
 from prometheus_client.multiprocess import MultiProcessCollector
 from starlette.requests import Request
 from starlette.responses import Response
-from starlette.types import ASGIApp
 
 from .middleware import PrometheusMiddleware
 
@@ -24,6 +24,6 @@ def metrics(request: Request) -> Response:
     return Response(generate_latest(registry), media_type=CONTENT_TYPE_LATEST)
 
 
-def init(app: ASGIApp):
+def init(app: FastAPI):
     app.add_middleware(PrometheusMiddleware)
     app.add_route("/metricsp", metrics)

quetz/pkgstores.py

@@ -116,7 +116,7 @@ def file_exists(self, channel: str, destination: str):
     def get_filemetadata(self, channel: str, src: str) -> Tuple[int, int, str]:
         """get file metadata: returns (file size, last modified time, etag)"""
 
-    @abc.abstractclassmethod
+    @abc.abstractmethod
     def cleanup_temp_files(self, channel: str, dry_run: bool = False):
         """clean up temporary `*.json{HASH}.[bz2|gz]` files from pkgstore"""
 

quetz/rest_models.py

@@ -37,7 +37,7 @@ class User(BaseUser):
 Profile.model_rebuild()
 
 
-Role = Field(None, pattern='owner|maintainer|member')
+Role = Field(pattern='owner|maintainer|member')
 
 
 class Member(BaseModel):
@@ -58,7 +58,7 @@ class MirrorMode(str, Enum):
 
 
 class ChannelBase(BaseModel):
-    name: str = Field(None, title='The name of the channel', max_length=50)
+    name: str = Field(title='The name of the channel', max_length=50)
     description: Optional[str] = Field(
         None, title='The description of the channel', max_length=300
     )
@@ -134,7 +134,7 @@ class ChannelMetadata(BaseModel):
 
 class Channel(ChannelBase):
     metadata: ChannelMetadata = Field(
-        default_factory=ChannelMetadata, title="channel metadata", examples={}
+        default_factory=ChannelMetadata, title="channel metadata", examples=[]
     )
 
     actions: Optional[List[ChannelActionEnum]] = Field(
@@ -160,8 +160,14 @@ def check_mirror_params(self) -> "Channel":
         return self
 
 
+class ChannelWithOptionalName(Channel):
+    name: Optional[str] = Field(  # type: ignore[assignment]
+        None, title='The name of the channel', max_length=50
+    )
+
+
 class ChannelMirrorBase(BaseModel):
-    url: str = Field(None, pattern="^(http|https)://.+")
+    url: str = Field(pattern="^(http|https)://.+")
     api_endpoint: Optional[str] = Field(None, pattern="^(http|https)://.+")
     metrics_endpoint: Optional[str] = Field(None, pattern="^(http|https)://.+")
     model_config = ConfigDict(from_attributes=True)
@@ -173,7 +179,7 @@ class ChannelMirror(ChannelMirrorBase):
 
 class Package(BaseModel):
     name: str = Field(
-        None, title='The name of package', max_length=1500, pattern=r'^[a-z0-9-_\.]*$'
+        title='The name of package', max_length=1500, pattern=r'^[a-z0-9-_\.]*$'
     )
     summary: Optional[str] = Field(None, title='The summary of the package')
     description: Optional[str] = Field(None, title='The description of the package')
@@ -201,18 +207,18 @@ class PackageRole(BaseModel):
 
 
 class PackageSearch(Package):
-    channel_name: str = Field(None, title='The channel this package belongs to')
+    channel_name: str = Field(title='The channel this package belongs to')
 
 
 class ChannelSearch(BaseModel):
-    name: str = Field(None, title='The name of the channel', max_length=1500)
+    name: str = Field(title='The name of the channel', max_length=1500)
     description: Optional[str] = Field(None, title='The description of the channel')
-    private: bool = Field(None, title='The visibility of the channel')
+    private: bool = Field(title='The visibility of the channel')
     model_config = ConfigDict(from_attributes=True)
 
 
 class PaginatedResponse(BaseModel, Generic[T]):
-    pagination: Pagination = Field(None, title="Pagination object")
+    pagination: Pagination = Field(title="Pagination object")
     result: List[T] = Field([], title="Result objects")
 
 

quetz/tasks/mirror.py

@@ -6,7 +6,7 @@
 from concurrent.futures import ThreadPoolExecutor
 from http.client import IncompleteRead
 from tempfile import SpooledTemporaryFile
-from typing import List
+from typing import List, Optional
 
 import requests
 from fastapi import HTTPException, status
@@ -285,8 +285,8 @@ def initial_sync_mirror(
     dao: Dao,
     pkgstore: PackageStore,
     auth: authorization.Rules,
-    includelist: List[str] = None,
-    excludelist: List[str] = None,
+    includelist: Optional[List[str]] = None,
+    excludelist: Optional[List[str]] = None,
     skip_errors: bool = True,
     use_repodata: bool = False,
 ):
@@ -528,8 +528,8 @@ def synchronize_packages(
     pkgstore: PackageStore,
     auth: authorization.Rules,
     session: requests.Session,
-    includelist: List[str] = None,
-    excludelist: List[str] = None,
+    includelist: Optional[List[str]] = None,
+    excludelist: Optional[List[str]] = None,
     use_repodata: bool = False,
 ):
     logger.info(f"executing synchronize_packages task in a process {os.getpid()}")

quetz/testing/fixtures.py

@@ -1,13 +1,17 @@
 import os
 import shutil
 import tempfile
-from typing import List
+from functools import partial
+from typing import Callable, List
 
 import pytest
 from alembic.command import upgrade as alembic_upgrade
+from alembic.config import Config as AlembicConfig
 from fastapi.testclient import TestClient
+from pytest_mock import MockerFixture
 
 import quetz
+from quetz import cli
 from quetz.cli import _alembic_config
 from quetz.config import Config
 from quetz.dao import Dao
@@ -81,6 +85,16 @@ def use_migrations() -> bool:
         )
 
 
+@pytest.fixture
+def run_migrations(
+    alembic_config: AlembicConfig,
+    database_url: str,
+    mocker: MockerFixture,
+) -> Callable[[], None]:
+    mocker.patch("quetz.cli._alembic_config", return_value=alembic_config)
+    return partial(cli._run_migrations, database_url)
+
+
 @pytest.fixture
 def sql_connection(engine):
     connection = engine.connect()

quetz/tests/test_cli.py

@@ -4,11 +4,13 @@
 import tempfile
 from multiprocessing import Process
 from pathlib import Path
+from typing import Callable
 from unittest import mock
 from unittest.mock import MagicMock
 
 import pytest
 import sqlalchemy as sa
+from alembic.config import Config as AlembicConfig
 from alembic.script import ScriptDirectory
 from pytest_mock.plugin import MockerFixture
 from typer.testing import CliRunner
@@ -148,13 +150,13 @@ def refresh_db(engine, database_url):
 
 
 def test_run_migrations(
-    config, sql_connection, engine, database_url, alembic_config, refresh_db
+    config, sql_connection, engine, refresh_db, run_migrations: Callable[[], None]
 ):
     db = sql_connection
     with pytest.raises(sa.exc.DatabaseError):
         db.execute(sa.text("SELECT * FROM users"))
 
-    cli._run_migrations(alembic_config=alembic_config)
+    run_migrations()
 
     db.execute(sa.text("SELECT * FROM users"))
 
@@ -240,10 +242,15 @@ def test_make_migrations_plugin(mocker, config, config_dir, dummy_migration_plug
 
 
 def test_make_migrations_plugin_with_alembic(
-    config, config_dir, dummy_migration_plugin: Path, alembic_config, engine
+    config,
+    config_dir,
+    dummy_migration_plugin: Path,
+    engine,
+    run_migrations: Callable[[], None],
+    alembic_config: AlembicConfig,
 ):
     # make sure db is up-to-date
-    cli._run_migrations(alembic_config=alembic_config)
+    run_migrations()
 
     alembic_config.set_main_option(
         "version_locations",
@@ -265,7 +272,7 @@ def test_make_migrations_plugin_with_alembic(
     assert migration_scripts
 
     # apply migrations
-    cli._run_migrations(alembic_config=alembic_config)
+    run_migrations()
 
     # add a new revision
 
@@ -342,7 +349,13 @@ def downgrade(): pass
 
 
 def test_multi_head(
-    config, config_dir, dummy_migration_plugin: Path, alembic_config, engine, refresh_db
+    config,
+    config_dir,
+    dummy_migration_plugin: Path,
+    alembic_config,
+    engine,
+    refresh_db,
+    run_migrations: Callable[[], None],
 ):
     quetz_migrations_path = Path(config_dir) / "migrations"
     quetz_versions_path = quetz_migrations_path / "versions"
@@ -365,7 +378,7 @@ def test_multi_head(
         " ".join(map(str, [plugin_versions_path, quetz_versions_path])),
     )
     alembic_config.set_main_option("script_location", str(quetz_migrations_path))
-    cli._run_migrations(alembic_config=alembic_config)
+    run_migrations()
 
     # initialize a plugin
     cli._make_migrations(
@@ -375,7 +388,7 @@ def test_multi_head(
         initialize=True,
         alembic_config=alembic_config,
     )
-    cli._run_migrations(alembic_config=alembic_config)
+    run_migrations()
 
     rev_file = next((plugin_versions_path).glob("*test_revision.py"))
     with open(rev_file) as fid:
@@ -394,7 +407,7 @@ def test_multi_head(
         message="test revision",
         alembic_config=alembic_config,
     )
-    cli._run_migrations(alembic_config=alembic_config)
+    run_migrations()
 
     rev_file = next((quetz_versions_path).glob("*test_revision.py"))
     with open(rev_file) as fid:
@@ -414,7 +427,7 @@ def test_multi_head(
         content = fid.read()
     assert f"down_revision = '{plugin_rev_1}'" in content
 
-    cli._run_migrations(alembic_config=alembic_config)
+    run_migrations()
 
     # check heads
 

setup.cfg

@@ -37,6 +37,7 @@ install_requires =
   pydantic>=2.0.0
   pyyaml
   requests
+  rq
   sqlalchemy
   sqlalchemy-utils
   tenacity
@@ -82,5 +83,24 @@ dev =
   pytest-cov
   pytest-timeout
   tbump
+  %(mypy)s
+mypy =
+  mypy
+  sqlalchemy-stubs
+  types-Jinja2
+  types-aiofiles
+  types-appdirs
+  types-click
+  types-mock
+  types-orjson
+  types-pkg-resources
+  types-pluggy
+  types-psycopg2
+  types-redis
+  types-requests
+  types-six
+  types-toml
+  types-ujson
+  types-urllib3
 test =
   %(dev)s