refactor(P4c): thread Config into file_transfer; delete the env-var bridge#273
Merged
Merged
Conversation
Structural refactor phase P4 (backend#796), first slice. Twenty modules (every validator + database + file_transfer) called setup_logging(config) at IMPORT time — reconfiguring the root logger's handlers globally, 20x on every import: an import-time side effect a library shouldn't have. The entrypoints (cli/run.main and the template scripts) already call setup_logging() at startup, so the per-module calls were redundant. Removes the import-time setup_logging(config) call + its now-unused import from all 20 modules. Each module's `config = Config()` and `logger.setLevel(config.LOG_LEVEL)` are kept (full config-by-construction + the env-var-bridge removal are the next P4 slices). Logging is now configured once, at the app entrypoint. Behaviour-preserving: root logging is still configured by the entrypoint; child loggers inherit. Full unit suite 1078 passed, 96.9% coverage; e2e (real MySQL) 23 passed, 1 xpassed — characterization goldens unchanged. Next: P4b (inject Config into the file validators) and P4c (file_transfer takes paths; delete the cli/run env-var bridge) — the config-by-construction core, now gateable with the local e2e harness. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tors Structural refactor phase P4 (backend#796), config-by-construction slice 1. The path-reading validators (FileType/Image/FilePairing/BIOLabel/XML/Tokenizer read SRC_PATH, Duplicate reads DEST_PATH, TableName reads TABLE_NAME) each read a module-global `config = Config()` that snapshots os.environ. That global is what the cli/run env-var bridge exists to feed; to remove that bridge (P4c) the validators must read the ingestor's explicitly-resolved Config instead. Adds a single injection seam rather than threading `config=` through the ~40 factory construction sites: - BaseValidator gains a class-level `_config = None` + `bind_config(config)`. - map_validators(category, options, config=None) binds the run's Config to every validator it builds. Optional, so direct callers/tests keep the prior module-global fallback. - BaseIngestor.validate_data passes self.database.config (the injected one). - Each path-reading site now reads `(self._config or config).X` — injected when bound, module-global otherwise. DuplicateValidator.dest_path became a lazy property so it resolves AFTER the bind (it read DEST_PATH in __init__). Behaviour-preserving: during P4b the bridge still sets env and the injected Config reads the same env, so both paths resolve identically — pure plumbing that lets P4c flip the source of paths from env to Config overrides. New tests pin the seam (injected Config wins over a divergent env-backed global; omitting it falls back). Full unit suite 1080 passed, 96.9% coverage; e2e (real MySQL) 23 passed, 1 xpassed — characterization goldens unchanged. Next: P4c — file_transfer takes the resolved paths and cli/run builds Config with overrides instead of writing os.environ (delete _set_legacy_env_vars). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ridge Structural refactor phase P4 (backend#796), config-by-construction slice 2 — the slice that actually retires the os.environ bridge. cli/run previously bridged the resolved ingest.yaml into the path layer by WRITING os.environ["SRC_PATH"/"TABLE_NAME"/"LABEL_FILE"], which a scattering of module-global `config = Config()` instances read lazily (spooky action through the process environment). This removes that bridge: - file_transfer: every copy primitive (_find_src, _find_mask_src, image / annotation / text / mask_transfer, _copy_tokenizer_if_present) and the map_file_transfer dispatcher take an optional `cfg` (the run's Config), reading SRC_PATH / DEST_PATH from it; None falls back to the module-global for direct callers / tests. modalities/transfer.py threads cfg through the 7 per-category factories to those primitives. - BaseIngestor.ingest passes self.database.config to map_file_transfer; _check_src_path takes the injected Config (it's a staticmethod, so a `config` param defaulting to None -> env keeps its direct-call tests working). - cli/run: `_set_legacy_env_vars` is DELETED. `_resolve_config(resolved)` now builds ONE Config(SRC_PATH=..., TABLE_NAME=..., LABEL_FILE=...) from the resolved YAML and injects it (Database -> ingestor -> validators (P4b) + file transfer). No os.environ mutation. ModalitySpec.transfer is now (record, options, cfg) -> record | None. Behaviour-preserving: e2e (real MySQL) 23 passed, 1 xpassed — the #247 characterization goldens (MySQL cell values, DEST_PATH file manifest, backend payloads) are byte-identical with paths now flowing by construction instead of through env. Two cli/run tests that asserted the old bridge behaviour are rewritten to assert config-by-construction + that os.environ is left untouched; the map_file_transfer mock lambdas in 4 test files gained the cfg arg. Full unit suite 1080 passed, 96.8% coverage. With P4a+P4b+P4c the env-var bridge is gone and Config flows by construction to validators and file transfer. (A few module-global `config = Config()` fallbacks remain as the direct-call/test default; removing them is a later cleanup once all call sites inject.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
👋 Heads-up — Code review queue is at 13 / 8 Above the WIP limit. The team convention is to review existing PRs before opening new work. Open PRs currently in Code review (oldest first):
Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.) |
This was referenced Jun 16, 2026
Base automatically changed from
refactor/p4b-inject-config-validators
to
develop
June 16, 2026 09:29
divyasinghds
previously approved these changes
Jun 16, 2026
…transfer-paths-bridge # Conflicts: # tracebloc_ingestor/ingestors/base.py
divyasinghds
dismissed
their stale review
The merge-base changed after approval.
divyasinghds
approved these changes
Jun 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Structural refactor — phase P4 (config injection), slice 2: the slice that actually retires the
os.environbridge. Stacked on #272 (P4b).cli/runpreviously bridged the resolvedingest.yamlinto the path layer by writingos.environ["SRC_PATH" / "TABLE_NAME" / "LABEL_FILE"], which a scattering of module-globalconfig = Config()instances read lazily — spooky action through the process environment. This removes that bridge and flowsConfigby construction instead.Changes
file_transfer— every copy primitive (_find_src,_find_mask_src,image_/annotation_/text_/mask_transfer,_copy_tokenizer_if_present) and themap_file_transferdispatcher take an optionalcfg(the run's Config) and readSRC_PATH/DEST_PATHfrom it.Nonefalls back to the module-global for direct callers/tests.modalities/transfer.py— threadscfgthrough the 7 per-category factories to those primitives.ModalitySpec.transferis now(record, options, cfg) -> record | None.BaseIngestor—ingestpassesself.database.configtomap_file_transfer;_check_src_pathtakes the injected Config (it's a@staticmethod, so aconfig=Noneparam keeps its direct-call tests working).cli/run—_set_legacy_env_varsis deleted._resolve_config(resolved)builds oneConfig(SRC_PATH=…, TABLE_NAME=…, LABEL_FILE=…)from the resolved YAML and injects it (Database→ ingestor → validators (P4b) + file transfer). Noos.environmutation.Behaviour preservation
This is the most path-sensitive slice, so it leans on the #247 characterization harness:
Two
cli/runtests that asserted the old bridge behaviour are rewritten to assert config-by-construction + thatos.environis left untouched; themap_file_transfermock lambdas in 4 test files gained thecfgarg.Net effect of P4
With P4a + P4b + P4c the env-var bridge is gone and
Configflows by construction to the validators and file transfer. A few module-globalconfig = Config()fallbacks remain as the direct-call/test default — removing them is a later cleanup once all call sites inject.🤖 Generated with Claude Code