Env vars override the YAML config file, as documented (#19)

CLAUDE.md

@@ -863,6 +863,23 @@ class MldpConfig(BaseSettings):
         )
 ```
 
+**YAML values enter through their own settings source, never as init kwargs** (#19;
+`plan/tickets/19/plan.md`).  `from_yaml()` flattens the file into a module-level `ContextVar`, calls
+`cls()`, and resets the var in `finally`; the private `_YamlValuesSource` reads it, and
+`settings_customise_sources` ranks it last (`init, env, dotenv, secrets, yaml`), so it sits just above
+field defaults.  Init kwargs are pydantic-settings' *top* priority, so passing file values as
+`cls(**values)` — what `from_yaml()` did through 1.16.0 — makes the file silently beat every `MLDP_*`
+variable.  Validation runs on the merged result, so an invalid YAML value that an env var overrides
+loads without error.  `load_config(config_object=)` returns the object as-is: explicit is level 1.
+`env_ignore_empty=True` makes an empty `MLDP_*` variable count as unset; once env outranked the file,
+an empty variable (a compose `${VAR}` with its source unset) would otherwise have blanked a host the
+file set.  `tests/unit/test_config.py::TestConfigPrecedence` pins all of this against real files.
+
+Any test that asserts a value from a YAML file or a default must clear ambient `MLDP_*` variables,
+since they now win: use `isolate_mldp_env(self)` in `setUp` or the `mldp_env()` context manager, both
+in `tests/unit/mldp_env.py`.  Otherwise the test passes in CI and fails in a developer shell that
+exports `MLDP_*` to point integration tests elsewhere.
+
 ### Key Configuration Classes
 - **`ServiceConfig`** - Individual service configuration (host, port, use_tls) with gRPC channel creation
 - **`MldpConfig`** - Main config container with flattened fields for environment variable support

doc/cookbook/connecting.md

@@ -11,7 +11,7 @@ See [API conventions](conventions.md) for the patterns every call shares once yo
 - [Model](#model) — one client, three services, three channels
 - [Configuration files](#configuration-files)
 - [Environment variables](#environment-variables)
-- [Configuration priority](#configuration-priority) — **and a bug to be aware of**
+- [Configuration priority](#configuration-priority)
 - [Sub-clients can be None](#sub-clients-can-be-none)
 - [TLS](#tls)
 - [Logging](#logging)
@@ -143,41 +143,30 @@ export MLDP_CONFIG_FILE=/etc/mldp/config.yaml
 Names are case-insensitive.  `USE_TLS` accepts the usual boolean spellings (`true`/`false`,
 `1`/`0`).
 
+A variable set to the empty string counts as unset: `MLDP_INGESTION_HOST=` falls through to the
+YAML file or the default rather than blanking the host.  This matters in docker compose, where
+`MLDP_INGESTION_HOST: ${INGESTION_HOST}` passes an empty string when `INGESTION_HOST` is not set.
+
 ## Configuration priority
 
-The intended order, highest first:
+The order, highest first:
 
 1. Explicit constructor parameters (channels, `config=`)
 2. Environment variables (`MLDP_*`)
 3. The YAML configuration file
 4. Built-in defaults
 
-> ### ⚠️ Known bug: YAML silently beats environment variables
->
-> **As of 1.15.0, levels 2 and 3 are inverted whenever the key is present in the YAML file.**
-> Tracked as [#19](https://github.com/osprey-dcs/dp-python-lib/issues/19).
->
-> A setting written in YAML **cannot be overridden** by its `MLDP_*` environment variable.  The
-> env var is ignored, silently — no warning, no error:
->
-> ```
-> # mldp-config.yaml contains:  ingestion: {host: localhost}
-> MLDP_INGESTION_HOST=prod.example.com  ->  resolves to "localhost"   (env ignored)
-> MLDP_INGESTION_PORT=443               ->  resolves to 443           (works: port absent from YAML)
-> ```
->
-> The rule is per-key: a key **absent** from the YAML file *is* overridable by its env var; a key
-> **present** in the file is not.
->
-> Cause: `MldpConfig.from_yaml()` passes YAML values as constructor keyword arguments, and in
-> pydantic-settings init kwargs outrank environment variables.
->
-> **Until this is fixed**, do not rely on env vars to override a deployed YAML file.  Either keep
-> the setting out of the YAML entirely, or point at a different file with `MLDP_CONFIG_FILE` (that
-> variable is read before the file is loaded, so it works as documented).
-
-This also applies to the auto-load path, since a `mldp-config.yaml` in the working directory is
-picked up automatically — which is how the surprise usually arrives.
+The order is per key: a YAML file can set the host while `MLDP_INGESTION_PORT` sets the port, and
+any key neither one sets takes its default.
+
+Level 1 applies only to the fields you actually passed.  `MldpConfig(ingestion_host="x")` pins the
+ingestion host, but the fields it leaves out were filled from `MLDP_*` variables or defaults when the
+object was built, exactly as for any other `MldpConfig()`.
+
+An auto-discovered `mldp-config.yaml` is still level 3, so environment variables override it just
+as they would an explicit `config_file=`.  Releases before the fix for
+[#19](https://github.com/osprey-dcs/dp-python-lib/issues/19) (1.16.0 and earlier) got this wrong:
+a key present in the YAML file silently ignored its `MLDP_*` variable.
 
 ## Sub-clients can be None
 

doc/release-notes/NEXT.md

@@ -28,6 +28,7 @@ person cutting the release has any reason to re-read.
 - [Release pages are the notes file, verbatim (#56)](#release-pages-are-the-notes-file-verbatim-issue-56)
 - [Type checking in CI (#30)](#type-checking-in-ci-issue-30)
 - [Ready for typed gRPC stubs (#61)](#ready-for-typed-grpc-stubs-issue-61)
+- [Environment variables override the config file (#19)](#environment-variables-override-the-config-file-issue-19)
 - [Cutting the release](#cutting-the-release)
 
 ---
@@ -84,6 +85,35 @@ Nothing about the library's behavior changes.
   but no `.pyi`.
 - **The `[dev]` extra now includes `types-grpcio`**, so `grpc` is type-checked rather than ignored.
 
+## Environment variables override the config file (Issue #19)
+
+**Silent behavior change.**  An `MLDP_*` environment variable now overrides the same setting in the
+YAML configuration file, as the documentation has always said it would.  Through 1.16.0 the file
+won whenever it contained the key, and the environment variable was ignored without a warning.
+
+Check before upgrading: **if you set an `MLDP_*` variable and your config file sets a *different*
+value for the same key, the client will now connect using the environment variable's value.**  No
+error is raised; the connection simply goes somewhere else.  This includes the `mldp-config.yaml`
+that is discovered automatically in the working directory or project root.  To keep the old
+behavior, unset the variable.
+
+Two smaller consequences of the same fix:
+
+- **An invalid value in the file no longer raises if an environment variable overrides it**
+  (`port: abc` with `MLDP_INGESTION_PORT=443` now loads, using 443).  An invalid value that is
+  actually used still raises the same `ValueError`.
+- **An `MLDP_*` variable set to the empty string now counts as unset**, falling through to the file
+  or the default.  Previously an empty `MLDP_INGESTION_HOST=` was lost to the file when the file set
+  the key, but blanked the host when it did not, and an empty port or `use_tls` the file did not set
+  raised.  Without this, the fix above would have let an empty variable, for example a docker
+  compose `${VAR}` whose source is unset, override a working file with a blank host.
+- **`load_config(config_object=...)` returns the object you passed** rather than a copy with the
+  same values.  An explicit object was already level 1, above environment variables; only its
+  identity changes.
+
+See [#19](https://github.com/osprey-dcs/dp-python-lib/issues/19) and the configuration priority
+section of [`doc/cookbook/connecting.md`](https://github.com/osprey-dcs/dp-python-lib/blob/main/doc/cookbook/connecting.md#configuration-priority).
+
 ## Installing
 
 ```bash

plan/tickets/19/plan.md

@@ -195,8 +195,13 @@ to two small modules, and the prototype below shows it works.
   separate usability issue; file it if wanted.
 - `.env` / secrets-dir support: not configured today, and the source order leaves room for it.
 - Any change to discovery order (`find_config_file`), which already behaves as documented.
-- Isolating the existing `test_config.py` tests from ambient `MLDP_*` variables (T6).  They predate this
-  ticket and pass on a clean shell and in CI; the new tests clear the environment themselves.
+- ~~Isolating the existing `test_config.py` tests from ambient `MLDP_*` variables (T6).  They predate this
+  ticket and pass on a clean shell and in CI; the new tests clear the environment themselves.~~
+  **Brought into scope 2026-09-24, in review of PR #63.**  The premise was incomplete: the fix itself
+  exposes a test that the YAML file used to shield (`test_from_yaml_valid`), so with `MLDP_*` exported the
+  suite went from 8 failures on `main` to 9.  All three config test classes now clear ambient variables in
+  `setUp` via `tests/unit/mldp_env.py`.  The same review added `env_ignore_empty=True`: once env outranks
+  the file, an empty `MLDP_*` variable would otherwise beat a working file with a blank host.
 
 ## Dependencies and sequencing
 

src/dp_python_lib/config/config.py

@@ -1,8 +1,28 @@
 import logging
+from contextvars import ContextVar
+from typing import Any
 
 import grpc
 from pydantic import BaseModel
-from pydantic_settings import BaseSettings, SettingsConfigDict
+from pydantic.fields import FieldInfo
+from pydantic_settings import BaseSettings, PydanticBaseSettingsSource, SettingsConfigDict
+
+# The flattened values of the YAML file being loaded by MldpConfig.from_yaml(), read by
+# _YamlValuesSource.  A ContextVar rather than a class attribute so concurrent loads in other
+# threads or asyncio tasks cannot see each other's values; from_yaml() resets it in a
+# `finally`, so a failed load cannot leak file values into a later plain MldpConfig().
+_yaml_values: ContextVar[dict[str, Any] | None] = ContextVar("_yaml_values", default=None)
+
+
+class _YamlValuesSource(PydanticBaseSettingsSource):
+    """Settings source supplying the YAML file's values, ranked below environment variables (issue #19)."""
+
+    def get_field_value(self, field: FieldInfo, field_name: str) -> tuple[Any, str, bool]:
+        # Required by the ABC; __call__ supplies every value at once, so this is never used.
+        return None, field_name, False
+
+    def __call__(self) -> dict[str, Any]:
+        return dict(_yaml_values.get() or {})
 
 
 class ServiceConfig(BaseModel):
@@ -47,7 +67,26 @@ class MldpConfig(BaseSettings):
     annotation_port: int = 50053
     annotation_use_tls: bool = False
 
-    model_config = SettingsConfigDict(env_prefix="MLDP_", case_sensitive=False)
+    # env_ignore_empty: an empty MLDP_* variable counts as unset, so it falls through to the YAML file or
+    # the default.  Without it, `export MLDP_INGESTION_HOST=` (or a compose `${VAR}` that expands to "")
+    # would beat the file with an empty host and connect nowhere, or fail to parse as a port.
+    model_config = SettingsConfigDict(env_prefix="MLDP_", case_sensitive=False, env_ignore_empty=True)
+
+    @classmethod
+    def settings_customise_sources(
+        cls,
+        settings_cls: type[BaseSettings],
+        init_settings: PydanticBaseSettingsSource,
+        env_settings: PydanticBaseSettingsSource,
+        dotenv_settings: PydanticBaseSettingsSource,
+        file_secret_settings: PydanticBaseSettingsSource,
+    ) -> tuple[PydanticBaseSettingsSource, ...]:
+        # Priority, high to low: explicit constructor arguments, MLDP_* environment variables,
+        # the YAML file, field defaults.  This is pydantic-settings' default order with the YAML
+        # source added last.  YAML values must never be passed as constructor arguments: init
+        # kwargs outrank every other source, which is how the file came to silently beat
+        # MLDP_* variables before issue #19.
+        return (init_settings, env_settings, dotenv_settings, file_secret_settings, _YamlValuesSource(settings_cls))
 
     @property
     def ingestion(self) -> ServiceConfig:
@@ -66,7 +105,11 @@ def annotation(self) -> ServiceConfig:
 
     @classmethod
     def from_yaml(cls, yaml_file: str) -> "MldpConfig":
-        """Load configuration from YAML file."""
+        """Load configuration from YAML file.
+
+        ``MLDP_*`` environment variables override values from the file; keys the file leaves
+        out fall back to the environment, then to the field defaults.
+        """
         import yaml
 
         logger = logging.getLogger(__name__)
@@ -86,7 +129,7 @@ def from_yaml(cls, yaml_file: str) -> "MldpConfig":
                 raise ValueError(f"expected a mapping at the top level, got {type(data).__name__}")
 
             # Convert nested YAML structure to flat fields
-            flat_data = {}
+            flat_data: dict[str, Any] = {}
 
             for service in ["ingestion", "query", "annotation"]:
                 service_config = data.get(service)
@@ -104,7 +147,11 @@ def from_yaml(cls, yaml_file: str) -> "MldpConfig":
                         logger.debug("Loaded %s_use_tls: %s", service, service_config["use_tls"])
 
             logger.debug("Successfully loaded configuration from YAML, creating MldpConfig instance")
-            return cls(**flat_data)
+            token = _yaml_values.set(flat_data)
+            try:
+                return cls()
+            finally:
+                _yaml_values.reset(token)
 
         except FileNotFoundError:
             logger.warning("YAML configuration file not found: %s, using defaults", yaml_file)

src/dp_python_lib/config/loader.py

@@ -82,21 +82,12 @@ def load_config(config_file: str | None = None, config_object: MldpConfig | None
         config_object is not None,
     )
 
-    # If explicit config object provided, use it (but still allow env var overrides)
-    if config_object:
-        logger.info("Using explicit config object with environment variable overrides")
-        # Create a new instance that will pick up environment variables
-        return MldpConfig(
-            ingestion_host=config_object.ingestion.host,
-            ingestion_port=config_object.ingestion.port,
-            ingestion_use_tls=config_object.ingestion.use_tls,
-            query_host=config_object.query.host,
-            query_port=config_object.query.port,
-            query_use_tls=config_object.query.use_tls,
-            annotation_host=config_object.annotation.host,
-            annotation_port=config_object.annotation.port,
-            annotation_use_tls=config_object.annotation.use_tls,
-        )
+    # An explicit config object is level 1: returned as-is, so environment variables cannot
+    # override the fields its caller set.  Fields the caller left out already took env values
+    # or defaults when the object was built.
+    if config_object is not None:
+        logger.info("Using explicit config object")
+        return config_object
 
     # Find and load from YAML file (if available)
     yaml_file = find_config_file(config_file)

tests/unit/mldp_env.py

@@ -0,0 +1,36 @@
+"""
+Test support for isolating configuration tests from ambient ``MLDP_*`` environment variables.
+
+A developer shell may export ``MLDP_*`` (to point integration tests at a remote ecosystem, say).  Since issue #19
+those variables override the YAML file, so any test asserting a value from a file or a default must run with them
+removed, or it passes in CI and fails on that developer's machine.
+"""
+
+import contextlib
+import os
+import unittest
+from collections.abc import Iterator
+from unittest.mock import patch
+
+
+def _without_mldp() -> dict[str, str]:
+    return {k: v for k, v in os.environ.items() if not k.upper().startswith("MLDP_")}
+
+
+@contextlib.contextmanager
+def mldp_env(**overrides: str) -> Iterator[None]:
+    """Run with every ambient ``MLDP_*`` variable removed, plus ``overrides``."""
+    env = _without_mldp()
+    env.update(overrides)
+    with patch.dict(os.environ, env, clear=True):
+        yield
+
+
+def isolate_mldp_env(test: unittest.TestCase) -> None:
+    """From a ``setUp``: remove every ambient ``MLDP_*`` variable for the duration of the test.
+
+    A test's own ``@patch.dict(os.environ, {...})`` still applies on top, since it is entered after ``setUp``.
+    """
+    patcher = patch.dict(os.environ, _without_mldp(), clear=True)
+    patcher.start()
+    test.addCleanup(patcher.stop)