Repository navigation
fix(mem_scheduler): tolerate absent optional fields in EnvConfigMixin.from_env (#2424) - #2492
Open
RerankerGuo wants to merge 1 commit into
Open
RerankerGuo wants to merge 1 commit into
RerankerGuo wants to merge 1 commit into
Conversation
….from_env (MemTensor#2424) EnvConfigMixin.from_env previously raised a bare `raise ValueError()` (empty message) whenever a field declared as `field: int | None = None` had no environment variable set — so any config with a plain optional field could not be built from the environment at all. The scheduler init path (`memos/configs/mem_scheduler.py` calls `RabbitMQConfig.from_env()`) was the most visible victim: `port: int | None` is exactly that shape, so it failed immediately. A secondary bug: `_parse_env_value` only parsed when the type was exactly `bool`/`int`/`float`, so an `Optional[int]` env var that *was* provided would have been left as a string instead of being cast. The fix: - from_env now skips fields with no env var entirely; only genuinely required fields (no default and not Optional) still raise, and the raised error carries the missing env var name and the word "missing" (the bug path's bare ValueError() is replaced). - _parse_env_value unwraps `Optional[X]` / `X | None` (using typing.get_args) so an Optional[int] env var parses as int. - A new module-level helper `_is_optional` keeps the Optional check in one place, since both from_env and _parse_env_value need it. New tests/mem_scheduler/test_env_config_mixin.py covers: - absent Optional[int] falls back to None (the bug's reproducer) - absent str falls back to the field default - present Optional[int] parses as int - bool env var still parses as bool (regression coverage) - missing required field still raises with the env var name in the message (so the empty ValueError() path is dead) Fixes MemTensor#2424.
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
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
EnvConfigMixin.from_envraised a bareraise ValueError()(empty message) whenever a field declared asfield: int | None = Nonehad no environment variable set — so any config with a plain optional field could not be built from the environment at all. The scheduler init path (memos/configs/mem_scheduler.pycallsRabbitMQConfig.from_env()) was the most visible victim:port: int | Noneis exactly that shape, so it failed immediately.Root cause
field_info.defaultisNoneforOptional[int] = None, so theelifnever fires and the bareraise ValueError()triggers for every absent optional field. The error also makes it impossible to tell which field caused the failure._parse_env_valuehad a secondary issue: it only parsed whentarget_type is bool/int/float, so anOptional[int]env var that was provided would have been left as a raw string instead of being cast.Fix
from_envnow skips fields with no env var entirely; only genuinely required fields (no default and notOptional) still raise, and the raised error carries the missing env var name and the word "missing" (replacing the empty-message bug)._parse_env_valueunwrapsOptional[X]/X | Noneviatyping.get_args, so anOptional[int]env var parses asint._is_optionalkeeps theOptionalcheck in one place, used by bothfrom_envand_parse_env_value.Tests
New
tests/mem_scheduler/test_env_config_mixin.py, 5 cases:test_absent_optional_int_field_falls_back_to_none— the bug reproducer (red against unpatchedmain)test_absent_optional_str_field_falls_back_to_default— same bug for non-Optional defaultstest_optional_int_env_var_parses_as_int— secondary symptom (Optional[int] gets cast)test_bool_env_var_parses_as_bool— regression coveragetest_missing_required_field_raises_named_value_error— genuinely required fields still raise, with a meaningful messageVerification
Ruff:
ruff format --checkclean on touched files;ruff check0 new findings vsmain(5 pre-existingBLE001/B008/UPfindings auto-fixed byruff check --fixduring this PR, 1 pre-existingTC009finding remains, both unrelated to this change).Out of scope
Closes #2424