fix: support setting logging.yml overrides via prefect config set#22411
fix: support setting logging.yml overrides via prefect config set#22411Abdulrehman-PIAIC80387 wants to merge 4 commits into
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: faa2573c02
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| elif setting_name in valid_logging_overrides: | ||
| parsed.add(_logging_override_setting(setting_name)) | ||
| else: | ||
| exit_with_error(f"Unknown setting name {setting_name!r}.") |
There was a problem hiding this comment.
Allow stored logging overrides to be unset
When a profile contains an override from a custom logging.yml and that file is later removed, changed, or the logging config path is unset, this validation rejects the key because it is no longer returned by _valid_logging_override_names() before checking the active profile. The stored PREFECT_LOGGING_* entry then cannot be removed with prefect config unset ..., leaving users to edit profiles.toml by hand; unset should also accept logging override names already present in profile.settings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b59ca3988 — config unset now also accepts any PREFECT_LOGGING_* key already stored in the active profile, so a stored override can always be removed even when it is no longer a valid override key (e.g. the logging config file changed). Added a regression test (test_unset_stored_logging_override_that_is_no_longer_valid).
05daecf to
3789f6f
Compare
desertaxle
left a comment
There was a problem hiding this comment.
Thanks for the PR @Abdulrehman-PIAIC80387! Looks like this mostly works based on some testing, but I think this needs one more fix before merge. The issue is that prefect config view doesn't show the effective value/source for custom logging overrides when the same PREFECT_LOGGING_* key is set in the environment. Runtime precedence is correct, but the CLI display is stale/misleading.
Would you be able to fix that in this PR?
|
Thanks @desertaxle! Fixed in
Added regression tests |
closes PrefectHQ#18666 Custom PREFECT_LOGGING_* keys that map into logging.yml (e.g. PREFECT_LOGGING_LOGGERS_PREFECT_FLOW_RUNS_LEVEL) are now first-class settings. A LoggingSettings.overrides field is populated by a new LoggingOverridesSource that collects unmatched PREFECT_LOGGING_* keys from the environment and profiles (cheap key filtering only, no logging.yml parsing on import). load_logging_config applies them via get_current_settings(), and the CLI accepts them in config set/unset/view.
…d keys Addresses review feedback: config unset now removes any stored PREFECT_LOGGING_* override from the profile even if it is no longer returned by the logging config key validation (e.g. the logging config file changed or was removed).
Memoize _declared_env_names per settings class so the LoggingOverridesSource does not re-walk the model fields on every settings build.
Addresses review feedback: config view now surfaces custom PREFECT_LOGGING_* overrides set via environment variables with the 'env' source, taking precedence over the profile value to match runtime resolution. Previously an override set in both the environment and a profile displayed the (stale) profile value, and an override set only in the environment was not shown at all.
02362a1 to
63a2aab
Compare
desertaxle
left a comment
There was a problem hiding this comment.
Thanks for the updates @Abdulrehman-PIAIC80387! I found a couple more edge cases while testing that should be resolved before this PR is merged.
| class LoggingOverridesSource(PydanticBaseSettingsSource): | ||
| """Collect `PREFECT_LOGGING_*` keys that do not map to a declared field into the | ||
| `overrides` field. | ||
|
|
||
| This lets logging configuration file (logging.yml) paths such as | ||
| `PREFECT_LOGGING_LOGGERS_PREFECT_FLOW_RUNS_LEVEL` be configured through the | ||
| settings system (environment variables and profiles) and show up in | ||
| `get_current_settings()` / `prefect config view`. | ||
|
|
||
| Only cheap key filtering is performed here; the logging configuration file is | ||
| never read or parsed at this layer. | ||
| """ | ||
|
|
||
| def __init__(self, settings_cls: Type[BaseSettings]): | ||
| super().__init__(settings_cls) | ||
| self.settings_cls = settings_cls | ||
| self._declared = _declared_env_names(settings_cls) | ||
|
|
||
| def get_field_value( | ||
| self, field: FieldInfo, field_name: str | ||
| ) -> Tuple[Any, str, bool]: | ||
| return None, field_name, False | ||
|
|
||
| def _is_override_key(self, key: str) -> bool: | ||
| return key.startswith("PREFECT_LOGGING_") and key not in self._declared | ||
|
|
||
| def _collect(self) -> Dict[str, str]: | ||
| overrides: Dict[str, str] = {} | ||
| # Lower priority: profile settings (config set writes here) | ||
| try: | ||
| profile_settings = ProfileSettingsTomlLoader( | ||
| self.settings_cls | ||
| ).profile_settings | ||
| except Exception: | ||
| profile_settings = {} | ||
| for key, value in profile_settings.items(): | ||
| upper = key.upper() | ||
| if self._is_override_key(upper): | ||
| overrides[upper] = str(value) | ||
| # Higher priority: environment variables override profile values | ||
| for key, value in os.environ.items(): | ||
| upper = key.upper() | ||
| if self._is_override_key(upper): | ||
| overrides[upper] = value | ||
| return overrides | ||
|
|
||
| def __call__(self) -> Dict[str, Any]: | ||
| overrides = self._collect() | ||
| return {"overrides": overrides} if overrides else {} |
There was a problem hiding this comment.
I think this should be split into two different classes because environment variables and profiles have two different priorities when resolving settings.
| dotenv_settings, | ||
| file_secret_settings, | ||
| ) | ||
| return (*sources, LoggingOverridesSource(settings_cls)) |
There was a problem hiding this comment.
Because the LoggingOverridesSource is appended to the end of the sources, configuration in a TOML file will override environment variables, which doesn't align with the existing convention.
I mentioned this in src/prefect/settings/sources.py, but this seems like a good reason to split LoggingOverridesSource into two different classes.
| # Custom logging overrides set via environment variables. The environment takes | ||
| # precedence over the profile at runtime, so surface it here first (later sources | ||
| # skip names already processed) to keep the displayed source accurate. | ||
| for env_name, env_value in os.environ.items(): | ||
| upper = env_name.upper() | ||
| if not upper.startswith("PREFECT_LOGGING_"): | ||
| continue | ||
| if upper in valid_setting_names or upper in processed_settings: | ||
| continue | ||
| if upper in valid_logging_overrides or upper in profile_override_names: | ||
| _process_setting(_logging_override_setting(upper), env_value, "env") |
There was a problem hiding this comment.
Looks like this handles logging config via env vars, but we'll also want to handle display from a prefect.toml source.
| names: set[str] = set(get_valid_setting_overrides(DEFAULT_LOGGING_SETTINGS_PATH)) | ||
| configured = get_current_settings().logging.config_path | ||
| if configured and Path(configured).exists(): | ||
| names |= get_valid_setting_overrides(Path(configured)) |
There was a problem hiding this comment.
This looks like it merges the default logging config with a custom logging config file if one exists, but at runtime we completely ignore the default file if a custom logging file is provided. Can you align this helper with the runtime behavior?
closes #18666
Root cause
prefect config setvalidates keys against registeredSettingsfields.PREFECT_LOGGING_LEVELmaps to a real field, butPREFECT_LOGGING_LOGGERS_PREFECT_FLOW_RUNS_LEVELis a dotted path intologging.yml, not a Pydantic field — so it's rejected withUnknown setting name. The runtime honors these keys only when read directly fromos.environ(inload_logging_config), which is why the env-var workaround works butconfig set(which writes to a profile) does not.Fix
Make custom
PREFECT_LOGGING_*keys first-class settings:LoggingSettings.overrides(settings/models/logging.py) — adict[str, str]holding override keys → values. Because it's a real field, the values integrate withget_current_settings()and render inprefect config view.LoggingOverridesSource(settings/sources.py) — collectsPREFECT_LOGGING_*keys that don't match a declared field, from the environment and the active profile, intooverrides. Cheap key filtering only — it never reads or parseslogging.yml.load_logging_config(logging/configuration.py) — applies overrides fromget_current_settings().logging.overrides(existingos.environbehavior preserved as the higher-priority source). This is lazy (runs atsetup_logging, not import).cli/config.py) —config set/unsetaccept logging-override keys (validated againstlogging.yml, parsed lazily and cached, CLI-only) and persist them asSettingobjects;config viewrenders them._cast_settings(settings/profiles.py) — wraps unknownPREFECT_LOGGING_*profile keys asSettingobjects (cheap prefix check) so they round-trip through profiles andconfig viewwithout warnings.How this avoids #18670's failure modes
logging.ymlis parsed lazily and only in the CLI. Guard test:test_collecting_overrides_does_not_parse_logging_config.config viewcrashed ('str' has no attribute 'name')Settingobjects, not raw strings. Test:test_view_shows_logging_override.load_logging_configreads fromget_current_settings().logging.overrides. Test:test_load_logging_config_applies_settings_overrides.Verification
Regression tests added in
tests/cli/test_config.py,tests/test_logging.py.tests/test_settings.pyupdated (SUPPORTED_SETTINGS, regenerated_types.py). Fulltests/test_settings.py,tests/cli/test_config.py,tests/test_logging.pypass; ruff clean; no new mypy errors.Open questions for maintainers
logging.overrides) — happy to rename.Settingwrappers in profiles vs. a dedicated mechanism — feedback welcome.Checklist
<link to issue>"mint.json.