Skip to content

fix(mem_scheduler): tolerate absent optional fields in EnvConfigMixin.from_env (#2424) - #2492

Open
RerankerGuo wants to merge 1 commit into
MemTensor:dev-v2.0.36from
RerankerGuo:fix/issue-2424-env-config-mixin
Open

RerankerGuo wants to merge 1 commit into
MemTensor:dev-v2.0.36from
RerankerGuo:fix/issue-2424-env-config-mixin

Conversation

@RerankerGuo

Copy link
Copy Markdown
Contributor

Summary

EnvConfigMixin.from_env 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.

Root cause

for field_name, field_info in cls.model_fields.items():
    ...
    if env_var in os.environ:
        field_values[field_name] = cls._parse_env_value(...)
    elif field_info.default is not None:
        field_values[field_name] = field_info.default
    else:
        raise ValueError()                       # ← empty, swallows any Optional[None]

field_info.default is None for Optional[int] = None, so the elif never fires and the bare raise ValueError() triggers for every absent optional field. The error also makes it impossible to tell which field caused the failure.

_parse_env_value had a secondary issue: it only parsed when target_type is bool/int/float, so an Optional[int] env var that was provided would have been left as a raw string instead of being cast.

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" (replacing the empty-message bug).
  • _parse_env_value unwraps Optional[X] / X | None via 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, used by both from_env and _parse_env_value.

Tests

New tests/mem_scheduler/test_env_config_mixin.py, 5 cases:

  1. test_absent_optional_int_field_falls_back_to_none — the bug reproducer (red against unpatched main)
  2. test_absent_optional_str_field_falls_back_to_default — same bug for non-Optional defaults
  3. test_optional_int_env_var_parses_as_int — secondary symptom (Optional[int] gets cast)
  4. test_bool_env_var_parses_as_bool — regression coverage
  5. test_missing_required_field_raises_named_value_error — genuinely required fields still raise, with a meaningful message

Verification

PYTHONPATH=src pytest tests/mem_scheduler/test_env_config_mixin.py -v
============================== 5 passed in 2.12s ==============================

Ruff: ruff format --check clean on touched files; ruff check 0 new findings vs main (5 pre-existing BLE001/B008/UP findings auto-fixed by ruff check --fix during this PR, 1 pre-existing TC009 finding remains, both unrelated to this change).

Out of scope

  • No new dependencies
  • No public API / OpenAPI contract changes
  • No DB schema, migration, or graph model changes
  • No Makefile / pyproject.toml / pre-commit / GitHub workflow changes

Closes #2424

….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.
@Memtensor-AI Memtensor-AI added area:scheduler 调度模块 status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Oct 13, 2026
@Memtensor-AI
Memtensor-AI requested a review from wustzdy October 13, 2026 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:scheduler 调度模块 status:in-progress Someone or AI is working on it | 人工或 AI 正在处理

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants