Skip to content

Fix recursive_update_dict to add new keys from project config#26

Open
nulinspiratie wants to merge 5 commits into
mainfrom
fix/recursive-update-dict-new-keys
Open

Fix recursive_update_dict to add new keys from project config#26
nulinspiratie wants to merge 5 commits into
mainfrom
fix/recursive-update-dict-new-keys

Conversation

@nulinspiratie

Copy link
Copy Markdown
Contributor

Summary

Fixes a bug in recursive_update_dict() that prevented project-specific configuration from adding new keys that didn't exist in the base config.

Problem

The recursive_update_dict() function in qualibrate_config/core/utils.py was skipping keys that didn't exist in the base dictionary:

for k, v in updates.items():
    if k not in to_update:
        continue  # ← BUG: Skips new keys instead of adding them!

This caused project configuration overrides to be silently ignored when they tried to add new configuration keys.

Real-World Impact: QUAM State Path

This bug particularly affected QUAM's state_path configuration:

Base config (~/.qualibrate/config.toml):

[quam]
raise_error_missing_reference = false
version = 3

[qualibrate]
project = "CS_1"

Project config (~/.qualibrate/projects/CS_1/config.toml):

[quam]
state_path = "/path/to/project/quam_state"

Expected behavior: Project's state_path should be merged into the final config
Actual behavior: state_path was silently skipped, causing QUAM to not recognize project-specific state paths

User Experience

When users switched projects and tried to load QUAM state:

from quam.config import get_quam_config
cfg = get_quam_config()
cfg.state_path  # ← Returns None instead of project-specific path!

This made project-based workflows impossible, as QUAM couldn't locate project-specific state files.

The Fix

Changed line 12 to add new keys instead of skipping them:

for k, v in updates.items():
    if k not in to_update:
        to_update[k] = v  # ← Now adds new keys
        continue
    # ... rest of merge logic

Testing

Verified the fix with:

  1. Unit tests: New keys are properly added during merge
  2. Integration test: Real config with CS_1 project now correctly returns project-specific state_path

Before fix:

cfg.state_path  # None

After fix:

cfg.state_path  # PosixPath('/path/to/project/quam_state')

Impact

  • ✅ Project-specific config overrides now work for all configuration keys
  • ✅ No changes to existing merge behavior for updating existing keys
  • ✅ No breaking changes to API

Related Issues

This completes the fix for project-aware QUAM configuration, which also required removing the @cache decorator from get_quam_config() in the QUAM repository.

Previously, recursive_update_dict would skip keys that didn't exist
in the base config, preventing project-specific configuration from
adding new keys. This caused project overrides to be ignored when
they introduced new configuration keys.

Example bug scenario:
- Base config: [quam] has version=3, no state_path
- Project config: [quam] adds state_path="/project/path"
- Result (before fix): state_path was SKIPPED
- Result (after fix): state_path is ADDED

The fix changes line 12 from "continue" to "to_update[k] = v; continue",
ensuring new keys are added while preserving the existing recursive
merge behavior for nested mappings.
Tests cover the specific bug scenario where project configs add new
keys that don't exist in the base config. The key test case is
test_recursive_update_dict_project_config_use_case which verifies
the real-world scenario:

- Base config has [quam] section with version, no state_path
- Project config adds [quam].state_path
- Result should include both base and project values

Additional test cases cover:
- Adding new keys at various nesting levels
- Updating existing keys
- Adding new top-level sections
- Nested merging with mixed new/existing keys
- Edge cases (empty updates, non-dict overrides)

All tests pass with the fix in recursive_update_dict.
@@ -10,6 +10,7 @@ def recursive_update_dict(
) -> RawConfigType:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain what this function is doing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added docstring, does this clarify it?

Comment thread tests/unit/test_core/test_utils.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants