Skip to content

chore(python): drop dead type: ignore - #122

Draft
fredj wants to merge 34 commits into
mainfrom
rm_extra_type_ignore
Draft

chore(python): drop dead type: ignore#122
fredj wants to merge 34 commits into
mainfrom
rm_extra_type_ignore

Conversation

@fredj

@fredj fredj commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Removes now-unnecessary # type: ignore comments (jwt, pydantic, pyproj, geoservercloud, geonetwork stubs). Rebased onto use-gdal; pandas/geopandas are already gone there, so the pandas-stubs/types-geopandas dependency addition was dropped.

@fredj
fredj force-pushed the rm_extra_type_ignore branch from da94b8d to 629b69f Compare August 31, 2026 12:44
f-necas and others added 29 commits September 7, 2026 14:39
The pandas removal dropped the dependency from apps/elt/uv.lock but left
process-dag-generator.py calling PostgresHook.get_pandas_df(). Since this
module runs at scheduler parse time, the ImportError would prevent every
scheduled ingestion DAG from being generated.

get_records_as_dicts() was already added to utils.py as the replacement
but was never wired up. Call it, and drop the now-unused
get_datafeeder_pg_hook import.

The test stubbed get_pandas_df, so it passed against the broken path;
stub the new helper instead so it exercises the real code.
Three call sites joined the full ogr2ogr argv and logged it at DEBUG,
directly above the comment warning that the command must never be logged.
The argv embeds the PG: connection string (database password) and, for OGC
services, GDAL_HTTP_USERPWD. Airflow task logs are readable from the web
UI, so enabling DEBUG exposed these secrets.

Remove the three logger.debug calls. The surrounding logger.info lines
already identify the source and target without any credential.

Add regression tests asserting no password reaches the log records for
the file, database-to-database and OGC paths.
Library code wrote ingestion timing to stdout with print(), bypassing the
configured logging and polluting worker output. Drop it along with the
time.time() scaffolding and the now-unused time import.
The airflow profile services build FROM the locally-built base image, which
is never published to a registry, so `make up` failed on a fresh checkout.
The target is already guarded by a `docker images -q` check, so it is a no-op
once the image exists.
response.content materialised the entire response body in memory before
writing it to the temp file, so a multi-GB source was fully resident in
RAM. This raised peak memory above the chunked geopandas reader it
replaced, contradicting the goal of handing files straight to GDAL.

Stream with stream=True and iter_content into the temp file. Headers are
still read before the body is consumed, so Content-Disposition filename
extraction is unchanged.

iter_content is used rather than copyfileobj(response.raw) because it
applies Content-Encoding, whereas raw would write compressed bytes.

Add tests covering the URL path, which had no coverage: one asserts the
body is streamed (the fake response raises on .content), the other that
Content-Disposition still names the temp file.
Zipped shapefiles were passed to ogr2ogr as a plain path, which GDAL
cannot open: ogr.Open("x.zip") fails with "not recognized as being in a
supported file format". Since main relied on fiona (which applies zip://
itself), handing the raw path to the ogr2ogr binary regressed a common
input format.

Resolve ZIPs to /vsizip/<archive>, appending the subdirectory when the
dataset is not at the archive root — /vsizip/<archive> alone fails for a
nested layout.

Reject archives holding more than one dataset. ogr2ogr -nln writes every
layer into the same table, so with -overwrite each layer replaced the
previous one and only the last survived, with a zero exit code and
nothing in the logs. The error names the datasets found so the user knows
which to extract. Shapefile sidecars (.shx/.dbf/.prj/...) are grouped
with their .shp so they don't count as separate datasets.

Verified against GDAL 3.12 for flat and nested archives; GeoPackage is
unaffected.
-a_srs was applied to every OGC ingestion. It relabels the CRS without
reprojecting, which is correct for OAPIF (GeoJSON is WGS84 lon/lat per
RFC 7946) but wrong for WFS: a WFS serves whatever srsName was
negotiated, commonly a projected CRS such as EPSG:2154.

Forcing 4326 on such a service tagged metric coordinates as degrees, so
the downstream ST_Transform placed the data far from its real location —
with no error and a zero exit code.

Restrict -a_srs to the ogcFeatures protocol and let GDAL keep the SRS the
WFS advertises. The WFS test asserted the old behaviour, so invert it and
add the matching OAPIF case.
CREATE TABLE AS copies data and column types but no indexes, so the GiST
index that GeoPandas' to_postgis used to create was lost. Final tables are
the ones published in GeoServer, so every bbox query (WMS/WFS) degraded to
a sequential scan — verified on 200k rows, where the planner switches from
Seq Scan to Bitmap Heap Scan once the index exists.

Recreate it as idx_<table>_<geom_col>, the name to_postgis used, keeping
the behaviour iso-functional. This is also the name that
POSTGIS_TABLE_NAME_MAX_LENGTH (54) is sized for: at the maximum table
length the index name is exactly 63 chars, PostgreSQL's identifier cap.

Only geographic results get an index; tq.geom_column is None for tabular
data.
main declared chardet in data_manipulation for the geopandas encoding
detection, which this branch replaces with ogr2ogr. Nothing imports it
anymore, so remove the declaration.

The rebase also left uv.lock referencing chardet from data-manipulation
without the matching [[package]] entry, which made `uv lock` fail to
parse the file. Drop those two stale references and relock.
Two ways an ogr2ogr failure went unreported:

- text=True decoded stderr as strict UTF-8, but ogr2ogr echoes the
  offending record when it rejects non-UTF-8 input. Decoding then raised
  UnicodeDecodeError and hid GDAL's actual message behind a Python
  traceback. Decode with errors="replace".

- ogr2ogr exits 0 even after aborting a layer translation ("ERROR 1: Non
  UTF-8 content found ... Terminating translation prematurely"), so
  check=True reported success while no table had been created. Scan
  stderr for GDAL error lines and raise. The regex is anchored at the
  start of a line so a path containing "error" does not match.

Also chain CalledProcessError with `from exc`, which was missing.
…t .cpg

The move to ogr2ogr dropped the chardet-based detection that main had
just hardened (PR #107). GDAL covers most of it natively — it reads the
.cpg sidecar and assumes UTF-8 otherwise — but a shapefile shipped
without a .cpg in a Western European codepage then makes ogr2ogr abort
with "Non UTF-8 content found", writing no table at all.

Detect that case only: sample the .dbf (directly or inside the ZIP, as
main did) and pass --config SHAPE_ENCODING. When a .cpg exists, or the
content is ASCII/UTF-8, defer to GDAL and add nothing.

Two details found while testing against a real PostGIS:

- chardet classifies a whole .dbf as binary because the fixed-width
  header drowns out the text, and the trailing 0x1A EOF marker alone is
  enough to make it give up. Feed it the record section with 0x1A
  stripped.
- on such short samples chardet cannot separate the Latin codepages and
  returns cp1250 for Western text, which decodes "ê" as "ę". Collapse
  those onto CP1252, what shapefiles without a .cpg overwhelmingly use
  here.

chardet returns as a data_manipulation dependency since it is imported
again.

Verified end to end: shapefile and zipped shapefile, with and without
.cpg, plus GeoJSON, all yield "Café"/"Forêt" in PostGIS.
The chunked WFS pagination that _wfs_json_output_format and
_wfs_geojson_chunk_url served was replaced by a single ogr2ogr call, and
CHUNK_SIZE lost its last caller when reads stopped going through pandas.

_wfs_json_output_format also referenced ET, whose
xml.etree.ElementTree import this branch had already removed, so calling
it would have raised NameError. Ruff flagged that (F821) alongside the
unused typing.Literal (F401), which together were failing lint on the
whole branch.

Removing them orphans os, Literal, urlencode and urlunparse, plus the
_WFS_JSON_FORMATS constant; drop those too. CHUNK_SIZE was neither
exported in __all__ nor referenced by any env file or documentation.
apps/elt/uv.lock still pinned apache-airflow-providers-fab, which this
branch removed from pyproject.toml when moving to Airflow 3.2.2. Relock
so the file matches its manifest.
`make test-libs` was red on main and use-gdal: 13 failures across three files,
all of them tests left behind by refactors rather than actual defects.

- test_utils: sanitize_name replaces hyphens with underscores (PostgreSQL
  identifiers reject them unquoted), so the two tests asserting hyphens are
  preserved were inverted. Its docstring contradicted its own examples and is
  fixed too.
- test_database: schema_exists/table_exists now go through sqlalchemy.inspect(),
  which rejects the MagicMock engine the tests injected via engine.connect().
  Patch inspect() instead.
- test_geoserver: create_workspace/create_layer moved to RestService
  (create_datastore with a DataStore, create_feature_type with a FeatureType);
  the tests still expected create_jndi_datastore/create_feature_type on the
  GeoServerCloud object. The non-geographic test also asserted inverted
  placeholder bounds.

Adds three cases covering the default-derivation branches that had none.
The datafeeder-gdal sidecar used by the LOCAL task executor ran
gdal:alpine-small, which ships no Parquet driver: ogrinfo on a .parquet fails
with "unable to open". The backend accepts parquet/geoparquet uploads
(FileType.PARQUET), and ingestion hands the file straight to ogr2ogr, so that
path was broken whenever TASK_EXECUTOR=LOCAL.

alpine-normal has the driver (523MB instead of 96MB). The Airflow image is
unaffected: it installs libgdal-arrow-parquet from conda-forge.
The comments claimed Trixie was needed because "bookworm only ships GDAL 3.6".
That justifies using conda-forge, not Trixie: no Debian release ships GDAL 3.13,
so conda-forge is required either way. The actual constraint is ABI: the
conda-forge build resolves libstdc++ as a direct NEEDED entry of the ogr2ogr
binary, outside its own RUNPATH, so it picks up the system copy. Bookworm's
libstdc++6 12.2 lacks GLIBCXX_3.4.31 / CXXABI_1.3.15 and ogr2ogr dies at exec
time; Trixie's 14.2 works. Neither LD_LIBRARY_PATH nor ldconfig avoids this,
both were tried.

The base image README was wrong on three further points:
- the vendored Dockerfile is not a verbatim copy: install_python() is rewritten
  to take Python from apt, with an early return leaving the upstream
  from-source build as dead code below it;
- so it does not compile Python from source, and AIRFLOW_PYTHON_VERSION is
  ignored (the documented 3.14.0 never applied; the image ships Trixie's 3.13.5);
- the patch list omitted that rewrite and described lcov as blocked by a
  from-source constraint that is no longer in effect.
The ELT shells out to ogr2ogr and needs GDAL >= 3.13, which no Debian release
packages. Getting it onto the official apache/airflow image meant installing
GDAL from conda-forge, and that build links against the *system* libstdc++:
it needs GLIBCXX_3.4.31 / CXXABI_1.3.15 (GCC 13+), which bookworm's 12.2 lacks,
so ogr2ogr installed fine but died at exec time. Neither LD_LIBRARY_PATH nor
ldconfig fixes that, because libstdc++ is a direct NEEDED entry of the binary,
resolved outside the conda prefix's RUNPATH. The workaround was a Trixie based
Airflow base image, built from a 2293-line vendored copy of the upstream Airflow
Dockerfile carrying three local patches.

Invert the layering instead: start from ghcr.io/osgeo/gdal:ubuntu-full, where
GDAL is native and glibc/libstdc++/GDAL are a coherent set, and install airflow
on top from apps/elt/uv.lock. The official image is still pulled, but only to
lift its /entrypoint and /clean-logs, which compose.airflow.yaml relies on.

This drops the vendored Dockerfile, its three patches, the base image build
target and its CI workflow. It also fixes the airflow build in
build-docker-images.yml, which ran `docker build` with no --build-arg and so
resolved AIRFLOW_BASE_IMAGE to a tag that is never published.

Notable consequences:

- celery and fab are now declared in apps/elt/pyproject.toml. Both are required
  by compose.airflow.yaml (CeleryExecutor, FabAuthManager) but were only ever
  present because the official image preinstalls 27 providers.
- Ubuntu 26.04 packages python3.14 only, so uv fetches the 3.13 interpreter the
  workspace pins. The four python pins are unchanged.
- /etc/passwd is made group-writable, as the official image does, so the
  entrypoint can register the arbitrary AIRFLOW_UID compose runs as. Without it
  getpass.getuser() fails and every airflow command dies.
- pip is installed in the venv because uv omits it and the entrypoint shells out
  to it for _PIP_ADDITIONAL_REQUIREMENTS.

Verified on the full compose stack: all airflow services healthy, staging_dag
ingesting a GeoJSON over HTTP into PostGIS through ogr2ogr (2 rows, correct
geometry and inferred types), Parquet read and write, and
_PIP_ADDITIONAL_REQUIREMENTS installing a package absent from the lock.
…r bbox

Neither is related to the image rework; both were found while testing it.

staging_dag and process_dag declared `default=""` on the callback URL params
alongside `minLength=1`, so the default itself failed validation and any trigger
that did not override both params was rejected:

    ValueError: Invalid input for param success_callback_url: '' should be non-empty

Use None, which the params' own `["null", "string"]` type already allows and
which callback.py already treats as "no callback" via its truthiness check.

create_layer built the non-geographic extent by hand and forwarded the caller's
bbox as-is, tagging latLonBoundingBox as EPSG:4326 while its coordinates were
still in `epsg`: only the is_geographic branch ran the reprojection. Derive both
extents through the same helpers in both cases, and hoist the placeholder into a
module constant so it stops being a mutable default argument.
The local executor guide pointed at `make up-airflow`, which does not exist:
the targets are `up` (Airflow included) and `up-no-airflow` (Airflow replaced
by the local executor). It also claimed `make up` no longer starts Airflow,
which is the opposite of what the target does.

The sidecar image was still documented as alpine-small, missed when
ed630f8 moved it to alpine-normal for the Parquet driver.
The Data Manipulation job only ran lint, format and type check, so its 140
tests were never executed in CI. That is how the 13 tests fixed in 55634fc
could stay red on main unnoticed.

The Airflow ELT job has the same gap, but adding it there also requires
declaring pytest in apps/elt's dev dependency group.
…s helpers

Rebasing onto main reverted two of its changes, since this branch predates them
and touches the same lines.

Dependabot moved main to apache-airflow 3.3.0 (15dc083) while this branch had
pinned 3.2.2. Take 3.3.0 and let uv resolve the celery/fab/postgres providers
that match it, instead of the versions pinned for 3.2.2.

The local_executor.py merge also kept main's chunked read_and_transform_data /
write_data_to_postgis loop, which git could apply cleanly but which calls
symbols this branch removes with the pandas helpers. Restore the PostGIS-native
transform_staging_to_final call, keeping main's synchronous structure.
Raised in review on PR #46.

libpq parses keyword/value connection strings by splitting on whitespace, so a
password containing a space was truncated and ogr2ogr refused to connect:

    missing "=" after "ss" in connection info string
    ERROR 1: PQconnectdb failed.

Quote every value, escaping backslashes and single quotes as libpq documents.

The emptiness filter was also ineffective: the f-string had already turned a
missing component into the literal "None", which is non-empty, so GDAL received
`host=None`. Filter on the value before formatting instead.

Verified against a live PostGIS with a role whose password contains spaces.
geonetwork-ui@2.10.0 declares `@ngx-translate/*: 16.x` as peer dependencies but
its code imports TRANSLATE_HTTP_LOADER_CONFIG, which only exists in 17. No
version satisfies both: 16 installs but fails to compile, 17 compiles but fails
to install. `npm ci` has been broken since 9c2b5f4 moved geonetwork-ui to the
2.10.0 release, and fc852d5 correctly bumped ngx-translate to 17 to keep the
build working — but that is what npm now refuses.

Override the peer range for geonetwork-ui only, reusing the versions declared in
dependencies so there is no duplicated version to keep in sync. Upstream already
fixed this (geonetwork/geonetwork-ui 5a2c7a915, peers set to 17.x); it is
published on the `dev` tag but not in any stable release yet, so this override
can go once a fixed version ships.

The lockfile churn is npm regenerating the tree: it rejects a hand-added
overrides key as out of sync, so a full `npm install` is required.

Verified: npm ci, lint, format:check and build all pass, where npm ci failed on
main. The 357 unit test failures are unrelated and pre-existing (identical count
on main): ngx-translate-testing imports FakeMissingTranslationHandler and
TranslateFakeCompiler, both removed in ngx-translate 17, and no released version
of that library has dropped them.
tonio and others added 5 commits September 7, 2026 16:48
The overrides commit regenerated package-lock.json with npm 10 (Node 22), but
the CI runs Node 24, whose npm 11 records the full set of optional
platform-specific binaries. `npm ci` therefore rejected the lockfile as out of
sync:

    Missing: @napi-rs/nice-darwin-arm64@1.1.1 from lock file
    Missing: @napi-rs/nice-android-arm-eabi@1.1.1 from lock file
    [...]

Relock with npm 11 to match .nvmrc and NODE_VERSION in frontend-checks.yml. No
dependency version changes: 141 platform binaries added (@esbuild/*, fsevents,
@emnapi/*), and the two entries npm ci was asking for.

Verified with Node 24 / npm 11: npm ci, lint, format:check and build all pass.
The 357 unit test failures are unchanged and unrelated (see bbeeea3).
The 357 failing specs all came from ngx-translate-testing, whose last release
is from June 2023: it instantiated TranslateService by hand with 9 positional
arguments, two of which (FakeMissingTranslationHandler, TranslateFakeCompiler)
no longer exist in @ngx-translate/core 17.

    FakeMissingTranslationHandler is not a constructor
    TranslateFakeCompiler is not a constructor

No published version of that package works with 17, so replace it with a local
module exposing the same withTranslations/withDefaultLanguage/withCompiler API.
The specs only needed their import line changed. It delegates to
TranslateModule.forRoot() rather than building the service by hand, so future
ngx-translate internals stay out of the way.

ngx-translate-messageformat-compiler needs no change: it declares ^17.0.0 and
uses none of the removed symbols.

Verified with Node 24 / npm 11: npm ci, lint, format:check, build pass, and the
unit tests go from 357 failed / 155 passed to 512 passed.
_resolve_zip_source counts the datasets in an archive to refuse the ones holding
several, since `ogr2ogr -nln` would write every layer into the same table and
each would overwrite the previous. The count grouped members on (directory,
stem) while skipping only known shapefile sidecar extensions, so the
`__MACOSX/._<name>` entries the Finder adds looked like a second dataset:

    Archive cities.zip contains multiple datasets (._cities, cities).

Skip archive metadata before counting. Verified on a shapefile built with GDAL:
the archive is now accepted and ogr2ogr extracts its 4 features, where ogrinfo
had always reported a single layer.

Two related false positives remain, an ArcGIS `.shp.xml` and a `README.txt`
alongside the shapefile: fixing those properly means counting the layers GDAL
reports rather than guessing from filenames.
Several # type: ignore comments across the backend and data_manipulation
lib and their tests were no longer needed for the underlying library
stubs (jwt, pydantic, pyproj, geoservercloud, geonetwork). pandas and
geopandas were already removed on use-gdal, so the pandas-stubs and
types-geopandas dependencies from the original commit are dropped as
dead weight.
@fredj fredj changed the title chore(python): add pandas-stubs and types-geopandas, drop dead type: ignore chore(python): drop dead type: ignore Sep 9, 2026
@fredj
fredj force-pushed the rm_extra_type_ignore branch from 629b69f to 1020393 Compare September 9, 2026 06:43
@fredj
fredj marked this pull request as draft September 9, 2026 06:43
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.

3 participants