feat!: Add _imports_ key and rename globals to imports

[ibro45]

Summary

• Add _imports_ key support in YAML configs to declare imports available to all expressions
• Rename Config parameter from globals to imports
• Remove undocumented _requires_ key that was never implemented

Usage

YAML-based imports:

_imports_:
  torch: torch
  np: numpy
  Path: pathlib.Path

device: "$torch.device('cuda')"
data: "$np.array([1, 2, 3])"

Python parameter:
config = Config(imports={"torch": "torch", "np": "numpy"})

Breaking Changes

• Config(globals=...) → Config(imports=...)
• requires key no longer recognized as special key

[coderabbitai]

Walkthrough

This PR replaces the previous globals/requires model with an imports mechanism. Config.init now accepts an imports parameter (internal _imports storage); a new _process_imports_key() consumes a YAML imports key and removes it from config data; parser/preprocessor now use imports as evaluation context; parse_overrides uses yaml.safe_load; requires was removed from non-arg handling and docs/tests updated to reference imports.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Review Config.init signature change (renamed parameter, new allow_missing flag) and caller impact.
• Inspect new _process_imports_key() behavior (string dotted paths, object values) and removal of the key from resolved config.
• Validate parse_overrides YAML parsing semantics and backwards-compatibility edge cases.
• Confirm parser/preprocessor now receive imports consistently (Parser(globals=self._imports)).
• Check items.Component non_arg_keys update (removed requires) and tests adjusted accordingly.
• Verify schema.special_keys update to include imports/args and remove requires.

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	Description provides clear summary, usage examples, and breaking changes, but omits required template sections like Related Issue and Type of Change checkboxes.	Complete the PR template: check 'Breaking change' box under Type of Change and add any related issue links.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title clearly summarizes the main changes: adding _imports_ key and renaming globals to imports.
Docstring Coverage	✅ Passed	Docstring coverage is 95.24% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch imports

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/sparkwheel/config.py (1)

708-752: Consider logging or warning on failed imports.

When optional_import fails (returns _LazyRaise placeholder), the code silently stores it. User will only see the error when the import is accessed in an expression.

This is acceptable for lazy error handling, but consider adding a debug log or docstring note about this behavior.

+        # Note: Failed imports return a lazy placeholder that raises on access.
+        # This provides clear error messages at expression evaluation time.
         for name, module_path in imports_config.items():

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 52b5947 and bfdbde8.

📒 Files selected for processing (9)

• docs/user-guide/advanced.md (2 hunks)
• docs/user-guide/basics.md (1 hunks)
• docs/user-guide/instantiation.md (0 hunks)
• src/sparkwheel/config.py (8 hunks)
• src/sparkwheel/items.py (1 hunks)
• src/sparkwheel/schema.py (1 hunks)
• tests/test_components.py (1 hunks)
• tests/test_config.py (1 hunks)
• tests/test_items.py (0 hunks)

💤 Files with no reviewable changes (2)

• docs/user-guide/instantiation.md
• tests/test_items.py

🧰 Additional context used

📓 Path-based instructions (2)

**/*.md

⚙️ CodeRabbit configuration file

Remember that documentation must be updated with the latest information.

Files:

• docs/user-guide/basics.md
• docs/user-guide/advanced.md

**/*.py

⚙️ CodeRabbit configuration file

Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.

Files:

• src/sparkwheel/schema.py
• tests/test_components.py
• src/sparkwheel/items.py
• tests/test_config.py
• src/sparkwheel/config.py

🧬 Code graph analysis (2)

tests/test_config.py (1)

src/sparkwheel/config.py (2)

• Config (115-835)
• resolve (577-668)

src/sparkwheel/config.py (3)

src/sparkwheel/utils/module.py (1)

• optional_import (149-200)

src/sparkwheel/preprocessor.py (1)

• Preprocessor (21-353)

src/sparkwheel/parser.py (2)

• Parser (12-100)
• parse (59-71)

🔇 Additional comments (12)

tests/test_components.py (1)

114-114: LGTM!

Assertion correctly validates _mode_ exclusion from kwargs, aligning with non_arg_keys update.

docs/user-guide/basics.md (1)

413-415: LGTM!

Documentation correctly adds _imports_ to special keys and links to Advanced Features for details.

src/sparkwheel/schema.py (1)

313-314: LGTM!

Special keys correctly updated to include _imports_ and _args_, consistent with Component.non_arg_keys.

src/sparkwheel/items.py (1)

126-126: LGTM!

Breaking change: _requires_ removed from non_arg_keys. Documented in PR objectives.

docs/user-guide/advanced.md (2)

121-121: LGTM!

_imports_ correctly documented as a special key.

---

292-342: LGTM!

Comprehensive documentation of the imports mechanism. Examples clearly show both YAML-based _imports_ and Python imports parameter approaches.

tests/test_config.py (2)

137-148: LGTM!

Tests properly renamed from globals to imports, verifying the new API.

---

150-229: LGTM!

Comprehensive test coverage for _imports_ key handling: basic usage, multiple modules, dotted paths, removal after processing, interaction with imports parameter, and graceful handling of invalid values.

src/sparkwheel/config.py (4)

151-156: LGTM!

Docstring correctly documents the renamed imports parameter.

---

158-206: LGTM!

Clean initialization of _imports with proper handling of string module paths vs pre-imported objects.

---

686-700: LGTM!

Correct placement of _process_imports_key() call before parsing to ensure imports are available to expressions.

---

838-876: LGTM!

Docstring accurately documents the switch from ast.literal_eval to yaml.safe_load, with clear examples of YAML syntax differences.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/sparkwheel/config.py (1)

686-753: Guard _process_imports_key against non‑dict roots to avoid edge‑case errors

Currently _process_imports_key assumes self._data behaves like a dict:

imports_key = "_imports_"
if imports_key not in self._data:
    return

imports_config = self._data.pop(imports_key)

If someone constructs Config with a non‑dict root (e.g., a list) and then calls resolve(), this can raise in surprising ways:

• For a list root that happens to contain the string "_imports_", imports_key in self._data is True, and self._data.pop(imports_key) will attempt list .pop() with a string index, raising TypeError.
• Even when "_imports_" is not present, the membership check is slightly odd on non‑mapping roots.

Given Config already allows non‑dict roots in some tests (e.g., Config("not a dict")), a small guard would make this more robust and keep _imports_ strictly a mapping‑root feature.

Suggested tweak:

     def _process_imports_key(self) -> None:
@@
-        imports_key = "_imports_"
-        if imports_key not in self._data:
-            return
+        imports_key = "_imports_"
+        # Only process _imports_ when the root is a mapping
+        if not isinstance(self._data, dict):
+            return
+        if imports_key not in self._data:
+            return

The rest of the logic (dict-type check on imports_config, dotted-path handling with optional_import, and support for non-string/module objects) looks solid and aligns with the new tests.

🧹 Nitpick comments (2)

tests/test_config.py (1)

150-257: _imports_ behavior is well covered; consider one alias‑collision test (optional)

The new TestConfigImports cases thoroughly exercise:

• basic module imports,
• dotted module/class paths,
• interaction with the imports constructor parameter,
• invalid _imports_ values,
• and non‑string/imported objects.

Optionally, you could add a test where the same alias is provided both via imports={...} and via _imports_ to lock in the precedence semantics (constructor vs YAML). Current implementation lets the YAML _imports_ override the existing entry, which seems reasonable but isn’t asserted anywhere.

src/sparkwheel/config.py (1)

842-877: YAML‑based parse_overrides behavior matches docs and tests; import location optional nit

The move to yaml.safe_load for value parsing is consistent with the updated docstring and the tests (YAML booleans, nulls, lists, dicts, and invalid YAML fallbacks). Operator handling (~key, =key=value, key=value) still behaves as expected.

Minor optional nit: you re-import yaml inside parse_overrides; since this may be called in a loop, consider a module‑level import to avoid repeated local imports, though in practice Python’s import caching already keeps overhead small.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between bfdbde8 and e985ae2.

📒 Files selected for processing (5)

• docs/user-guide/advanced.md (2 hunks)
• docs/user-guide/basics.md (1 hunks)
• docs/user-guide/instantiation.md (0 hunks)
• src/sparkwheel/config.py (8 hunks)
• tests/test_config.py (1 hunks)

💤 Files with no reviewable changes (1)

• docs/user-guide/instantiation.md

🚧 Files skipped from review as they are similar to previous changes (2)

• docs/user-guide/basics.md
• docs/user-guide/advanced.md

🧰 Additional context used

📓 Path-based instructions (1)

**/*.py

⚙️ CodeRabbit configuration file

Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.

Files:

• tests/test_config.py
• src/sparkwheel/config.py

🧬 Code graph analysis (1)

src/sparkwheel/config.py (3)

src/sparkwheel/utils/module.py (1)

• optional_import (149-200)

src/sparkwheel/preprocessor.py (1)

• Preprocessor (21-353)

src/sparkwheel/parser.py (2)

• Parser (12-100)
• parse (59-71)

🔇 Additional comments (3)

tests/test_config.py (1)

137-148: Init tests for imports parameter are appropriate and low-coupling

Using "pandas" only to check population of parser._imports (without asserting import success) keeps tests independent of optional heavy dependencies, and the callable case verifies identity correctly. No changes needed.

src/sparkwheel/config.py (2)

150-207: imports initialization and wiring into Preprocessor look correct

The new imports kwarg is plumbed cleanly:

• self._imports is always a dict, and only dict inputs are processed, so bad types don’t leak in.
• String values go through optional_import, non‑strings are taken as-is (modules/callables), which matches the tests’ expectations.
• Passing self._imports into Preprocessor keeps the API aligned with the existing globals notion and is forward‑compatible even though it’s unused today.

No obvious correctness issues here.

---

700-701: Passing _imports into Parser correctly exposes YAML/imports to expressions

Switching Parser(globals=self._imports, ...) ensures both constructor‑level imports and YAML _imports_ end up in the expression evaluation context. Given the comprehensive tests around json/os/pathlib/Counter imports, this wiring looks correct.