johnsturgeon/tgfp-web

Watch

Fork 0

Normalize the import namespace — `app/` is currently imported under two names #371

New issue

Closed

opened 2026-08-13 18:06:15 +02:00 by johnsturgeon · 0 comments

johnsturgeon commented

2026-08-13 18:06:15 +02:00

Owner

Prerequisite for the uv / pyproject.toml migration (#343). Split out because
it is a defect fix, not a build change, and it should be reviewable on its
own.

Pure-Python change. Touches no Dockerfile, no compose file, no migration.

The defect

app/ is a package (app/__init__.py exists, empty), but it is also on
sys.path as a root — so its submodules get imported under two different names
in the same process:

# app/main.py
from models import Player, ...          # /app on sys.path
from config import Config               # /app on sys.path
from app.routers import auth, ...       # /  on sys.path, app as package
from app.dependency import ...          # /  on sys.path

Verified:

db module file      : .../app/db/__init__.py
app.db module file  : .../app/db/__init__.py
SAME MODULE OBJECT? : False
SAME ENGINE OBJECT? : False
SAME Config CLASS?  : False

Same file on disk, two entries in sys.modules, two sets of module-level state.

How both roots end up on the path

Context	Mechanism
Production container	`compose.prod.yml:13` sets `PYTHONPATH: /` while `WORKDIR` is `/app`
Local dev	script dir `app/` is auto-added; PyCharm's default "add content roots to PYTHONPATH" adds the repo root
pylint	`.pylintrc:13` — `init-hook='import sys; sys.path[0:0] = [".", "app"]'`

That .pylintrc line is why this has never been reported: lint is configured to
make both forms resolve.

Live consequences

Two database engines. app/db/__init__.py:24 creates engine at module
scope. Nine modules do from db import engine; app/jobs/sync_team_records.py:4
does from app.db import engine. That job runs against its own engine and its
own connection pool.
Config.get_config() runs twice — two load_dotenv() calls, two full
os.environ reads, two unrelated Config dataclasses.
Two copies of the ESPN client. app/models/model_helpers.py:4 uses
from app.espn_nfl import ...; create_picks.py, update_game.py and
sync_team_records.py use from espn_nfl import ....
isinstance across the boundary silently returns False. Not exercised
today, but it is a live trap for anyone adding a type check.
Hot reload amplifies it. app/main.py:190 runs
uvicorn.run("app.main:app", reload=True) while the initial script import was
bare main, so the dev loop imports the tree under both names on every reload.

Which name wins: `app.`-qualified

The package form is already the de-facto canonical one — it is the bare form
that is the intruder:

alembic/env.py:10 — import app.models (migrations already depend on it)
app/main.py:190 — uvicorn.run("app.main:app", ...)

It is also the only form that survives becoming an installed package in #343.

Direction of travel: 51 bare imports gain an app. prefix. The 8 already
using app. stay as they are. Intra-package relative imports (from .base import ..., from .model_helpers import ...) are correct and unchanged.

Scope

1. Prefix the 51 first-party imports

File	Lines
`app/dependency.py`	7, 8
`app/jobs/award_notify_discord.py`	6, 7
`app/jobs/award_update_all.py`	11, 12, 13, 14, 15
`app/jobs/create_picks.py`	12, 13, 14, 15
`app/jobs/nag_players.py`	17, 18, 19, 20
`app/jobs/scheduler.py`	10, 11, 12, 16
`app/jobs/sync_team_records.py`	1, 2
`app/jobs/update_all_scores.py`	5, 6, 8, 9
`app/jobs/update_game.py`	10, 11, 12
`app/jobs/update_player_records.py`	3, 4
`app/main.py`	22, 23, 24, 25, 30
`app/models/award_helpers.py`	10, 13, 14, 15
`app/routers/admin.py`	9, 10, 11, 12, 13, 14
`app/routers/auth.py`	5, 10
`app/routers/mail.py`	10, 11

Affected top-level names: config, db, dependency, espn_nfl, jobs,
models, routers.

2. Make pylint enforce it

.pylintrc:13:

-init-hook='import sys; sys.path[0:0] = [".", "app"]'
+init-hook='import sys; sys.path[0:0] = ["."]'

This is the regression guard as well as the cleanup: with app off the path,
any future bare first-party import fails lint as import-error instead of
silently creating a duplicate module.

3. Make template/static paths independent of the working directory

Three sites resolve relative to CWD, which is why the app only boots from
app/ and raises RuntimeError: Directory 'static' does not exist from
anywhere else:

File	Line	Current
`app/main.py`	137	`StaticFiles(directory="static")`
`app/main.py`	138	`Jinja2Templates(directory="templates")`
`app/routers/admin.py`	16	`Jinja2Templates(directory="templates")`

Use the pattern already established at app/routers/mail.py:23:

template_folder: Path = Path(__file__).parent.parent / "templates"

Not strictly required to fix the double import, but included here because the
container layout work in #370 depends on the app being launchable from a
directory other than app/, and because CWD-dependence is the same class of
fragility.

Explicitly out of scope

No Dockerfile or compose changes. PYTHONPATH: / and WORKDIR /app can
stay; once no code uses the bare form, the redundant path entry is inert. It
gets removed in #343 when the project becomes an installed package.
No pyproject.toml. That is #343.
No restructuring of app/ itself — no files move, no __init__.py
contents change.

Acceptance criteria

No first-party import resolves without the app. prefix
.pylintrc no longer injects app onto sys.path
pylint $(git ls-files '*.py') reports 10.00/10
App boots and serves /, /login, /ping from the repo root as well
as from app/
alembic current still resolves (env.py's import app.models unaffected)
Scheduler starts and sync_team_records runs against the shared engine

Verification

The duplication check, which must flip to all-True:

cd app && set -a && . ../config/.env.development && set +a && \
PYTHONPATH="$(git rev-parse --show-toplevel)" ../.venv/bin/python -c "
import app.db, app.config
import sys
dupes = [n for n in sys.modules
         if n in ('db','config','models','jobs','espn_nfl','routers','dependency')]
print('bare first-party modules loaded:', dupes or 'none')
assert not dupes, 'still importing under two namespaces'
print('OK — single namespace')
"

Then a grep that must return nothing:

grep -rnE "^(from|import) (models|config|db|jobs|espn_nfl|routers|dependency)\b" \
  app --include="*.py"

Repo has no test suite, so verification is lint plus a manual boot and an
exercise of the scheduler jobs.

Prerequisite for the `uv` / `pyproject.toml` migration (#343). Split out because it is a **defect fix**, not a build change, and it should be reviewable on its own. Pure-Python change. Touches no Dockerfile, no compose file, no migration. ## The defect `app/` is a package (`app/__init__.py` exists, empty), but it is *also* on `sys.path` as a root — so its submodules get imported under two different names in the same process: ```python # app/main.py from models import Player, ... # /app on sys.path from config import Config # /app on sys.path from app.routers import auth, ... # / on sys.path, app as package from app.dependency import ... # / on sys.path ``` Verified: ``` db module file : .../app/db/__init__.py app.db module file : .../app/db/__init__.py SAME MODULE OBJECT? : False SAME ENGINE OBJECT? : False SAME Config CLASS? : False ``` Same file on disk, two entries in `sys.modules`, two sets of module-level state. ### How both roots end up on the path | Context | Mechanism | |---|---| | Production container | `compose.prod.yml:13` sets `PYTHONPATH: /` while `WORKDIR` is `/app` | | Local dev | script dir `app/` is auto-added; PyCharm's default "add content roots to PYTHONPATH" adds the repo root | | pylint | `.pylintrc:13` — `init-hook='import sys; sys.path[0:0] = [".", "app"]'` | That `.pylintrc` line is why this has never been reported: lint is configured to make both forms resolve. ### Live consequences - **Two database engines.** `app/db/__init__.py:24` creates `engine` at module scope. Nine modules do `from db import engine`; `app/jobs/sync_team_records.py:4` does `from app.db import engine`. That job runs against its own engine and its own connection pool. - **`Config.get_config()` runs twice** — two `load_dotenv()` calls, two full `os.environ` reads, two unrelated `Config` dataclasses. - **Two copies of the ESPN client.** `app/models/model_helpers.py:4` uses `from app.espn_nfl import ...`; `create_picks.py`, `update_game.py` and `sync_team_records.py` use `from espn_nfl import ...`. - **`isinstance` across the boundary silently returns `False`.** Not exercised today, but it is a live trap for anyone adding a type check. - **Hot reload amplifies it.** `app/main.py:190` runs `uvicorn.run("app.main:app", reload=True)` while the initial script import was bare `main`, so the dev loop imports the tree under both names on every reload. --- ## Which name wins: `app.`-qualified The package form is already the de-facto canonical one — it is the bare form that is the intruder: - `alembic/env.py:10` — `import app.models` (migrations already depend on it) - `app/main.py:190` — `uvicorn.run("app.main:app", ...)` It is also the only form that survives becoming an installed package in #343. Direction of travel: **51 bare imports gain an `app.` prefix.** The 8 already using `app.` stay as they are. Intra-package relative imports (`from .base import ...`, `from .model_helpers import ...`) are correct and unchanged. --- ## Scope ### 1. Prefix the 51 first-party imports | File | Lines | |---|---| | `app/dependency.py` | 7, 8 | | `app/jobs/award_notify_discord.py` | 6, 7 | | `app/jobs/award_update_all.py` | 11, 12, 13, 14, 15 | | `app/jobs/create_picks.py` | 12, 13, 14, 15 | | `app/jobs/nag_players.py` | 17, 18, 19, 20 | | `app/jobs/scheduler.py` | 10, 11, 12, 16 | | `app/jobs/sync_team_records.py` | 1, 2 | | `app/jobs/update_all_scores.py` | 5, 6, 8, 9 | | `app/jobs/update_game.py` | 10, 11, 12 | | `app/jobs/update_player_records.py` | 3, 4 | | `app/main.py` | 22, 23, 24, 25, 30 | | `app/models/award_helpers.py` | 10, 13, 14, 15 | | `app/routers/admin.py` | 9, 10, 11, 12, 13, 14 | | `app/routers/auth.py` | 5, 10 | | `app/routers/mail.py` | 10, 11 | Affected top-level names: `config`, `db`, `dependency`, `espn_nfl`, `jobs`, `models`, `routers`. ### 2. Make pylint enforce it `.pylintrc:13`: ```diff -init-hook='import sys; sys.path[0:0] = [".", "app"]' +init-hook='import sys; sys.path[0:0] = ["."]' ``` This is the regression guard as well as the cleanup: with `app` off the path, any future bare first-party import fails lint as `import-error` instead of silently creating a duplicate module. ### 3. Make template/static paths independent of the working directory Three sites resolve relative to CWD, which is why the app only boots from `app/` and raises `RuntimeError: Directory 'static' does not exist` from anywhere else: | File | Line | Current | |---|---|---| | `app/main.py` | 137 | `StaticFiles(directory="static")` | | `app/main.py` | 138 | `Jinja2Templates(directory="templates")` | | `app/routers/admin.py` | 16 | `Jinja2Templates(directory="templates")` | Use the pattern already established at `app/routers/mail.py:23`: ```python template_folder: Path = Path(__file__).parent.parent / "templates" ``` Not strictly required to fix the double import, but included here because the container layout work in #370 depends on the app being launchable from a directory other than `app/`, and because CWD-dependence is the same class of fragility. --- ## Explicitly out of scope - **No Dockerfile or compose changes.** `PYTHONPATH: /` and `WORKDIR /app` can stay; once no code uses the bare form, the redundant path entry is inert. It gets removed in #343 when the project becomes an installed package. - **No `pyproject.toml`.** That is #343. - **No restructuring of `app/` itself** — no files move, no `__init__.py` contents change. --- ## Acceptance criteria - [ ] No first-party import resolves without the `app.` prefix - [ ] `.pylintrc` no longer injects `app` onto `sys.path` - [ ] `pylint $(git ls-files '*.py')` reports 10.00/10 - [ ] App boots and serves `/`, `/login`, `/ping` **from the repo root** as well as from `app/` - [ ] `alembic current` still resolves (env.py's `import app.models` unaffected) - [ ] Scheduler starts and `sync_team_records` runs against the shared engine ## Verification The duplication check, which must flip to all-`True`: ```bash cd app && set -a && . ../config/.env.development && set +a && \ PYTHONPATH="$(git rev-parse --show-toplevel)" ../.venv/bin/python -c " import app.db, app.config import sys dupes = [n for n in sys.modules if n in ('db','config','models','jobs','espn_nfl','routers','dependency')] print('bare first-party modules loaded:', dupes or 'none') assert not dupes, 'still importing under two namespaces' print('OK — single namespace') " ``` Then a grep that must return nothing: ```bash grep -rnE "^(from|import) (models|config|db|jobs|espn_nfl|routers|dependency)\b" \ app --include="*.py" ``` Repo has no test suite, so verification is lint plus a manual boot and an exercise of the scheduler jobs.

johnsturgeon referenced this issue from a commit

2026-08-13 19:34:00 +02:00

Normalize first-party imports to the app. namespace

johnsturgeon referenced this issue

2026-08-13 19:34:49 +02:00

Normalize first-party imports to the app. namespace #372

johnsturgeon referenced this issue from a commit

2026-08-13 20:14:09 +02:00

Close #371 — import namespace normalized (PR #372)

johnsturgeon closed this issue

2026-08-13 20:14:09 +02:00