Support explicit callable column dependencies in dataset schemas

[cdreetz]

Summary

• add explicit dependency support for plain callable columns via depends_on(...)
• support tuple syntax for callables: (callable_fn, ["dep_col"])
• include these explicit deps in the dependency graph/topological ordering
• export depends_on from package root and document usage in README
• add tests covering dependency graph + generation for both callable patterns

Testing

• uv run pytest -q tests/test_dataset_comprehensive.py tests/test_datset.py

[claude]

Pull Request Review: Support Explicit Callable Column Dependencies

Summary

This PR adds support for explicit dependency declarations for plain callable columns, allowing users to specify dependencies when automatic detection isn't possible. The implementation is well-designed and includes comprehensive test coverage.

✅ Strengths

1. Clean API Design

The PR provides two intuitive ways to declare dependencies:

• depends_on(callable, "dep1", "dep2") - functional wrapper approach
• (callable, ["dep1", "dep2"]) - tuple syntax for inline usage

Both approaches are well-documented in the README with clear examples.

2. Backwards Compatibility

The changes maintain full backwards compatibility:

• Existing implicit dependency detection via prompt templates still works
• New explicit dependencies extend rather than replace existing functionality
• Static methods handle both old and new patterns gracefully

3. Comprehensive Test Coverage

The PR includes excellent test coverage:

• Dependency graph construction tests (lines 135-147)
• Both wrapper and tuple syntax tested (lines 226-254)
• Integration with existing dependency resolution

4. Proper Dependency Merging

The implementation correctly merges explicit and implicit dependencies (dataset.py:188-190), ensuring both types work together without conflicts.

🔍 Code Quality Observations

Positive Aspects

1. Type hints added - The PR adds proper typing imports (Callable, Iterable)
2. Duplicate removal - Lines 199-203 properly handle duplicate dependencies while preserving order
3. Robust extraction - _extract_explicit_dependencies handles multiple input formats gracefully
4. Clean separation - _resolve_column_callable cleanly separates the unwrapping logic

Minor Suggestions

1. Type Hint for DependentCallable.__call__

The __call__ method could use a return type hint:

# Current (line 331)
def __call__(self, context: Dict[str, Any]) -> Any:
    return self.func(context)

# Suggested - already correct, but could be more specific if func signature is known
# This is fine as-is given the generic nature

2. Validation for Tuple Syntax

Consider adding validation to ensure the tuple has exactly 2 elements and the second is a list:

# In _extract_explicit_dependencies (lines 233-237)
elif (
    isinstance(func, tuple)
    and len(func) == 2
    and callable(func[0])
    and isinstance(func[1], (list, tuple, str))  # More explicit validation
):
    deps = func[1]

3. Edge Case: Empty Dependency List

The code handles None but could explicitly handle empty lists:

# In _extract_explicit_dependencies (line 243-244)
if deps is None or (isinstance(deps, (list, tuple)) and len(deps) == 0):
    return []

🐛 Potential Issues

1. Type Safety for Tuple Syntax (Low severity)

At dataset.py:233-237, the tuple syntax doesn't validate that func[1] is actually a list/iterable of strings. A malformed tuple like (callable, "not_a_list") might cause issues.

Recommendation: Add explicit type checking:

elif (
    isinstance(func, tuple)
    and len(func) == 2
    and callable(func[0])
    and isinstance(func[1], (list, tuple))  # Validate it's a collection
):
    deps = func[1]

2. Iterable Check May Be Too Broad (Low severity)

At dataset.py:247-248, the check isinstance(deps, Iterable) will match strings, which could lead to unexpected behavior if someone passes a single string instead of a list.

Current behavior:

# This would work but iterate over characters
depends_on(lambda ctx: ..., "col1")  # Expected: ["col1"]
# But if deps="col1", line 248 would create ["c", "o", "l", "1"]

However, this is mitigated by line 245 handling strings separately, so this is actually fine.

3. Missing Validation for Invalid Dependencies (Medium severity)

The code filters out dependencies not in the schema (dataset.py:206), but doesn't warn users when they declare a dependency that doesn't exist. This could lead to silent failures.

Recommendation: Consider adding a warning or raising an error:

# In _build_dependency_graph
for dep in ordered_deps:
    if dep not in self.schema:
        import warnings
        warnings.warn(f"Column '{column}' declares dependency on '{dep}' which is not in schema")

🔒 Security Considerations

No security concerns identified. The code:

• Doesn't execute untrusted code beyond what's already in the schema
• Properly validates circular dependencies
• Maintains the same security posture as existing code

⚡ Performance Considerations

• The changes add minimal overhead to dependency graph construction
• Static methods are efficient and don't impact runtime performance
• Dependency graph is built once per dataset generation, so performance impact is negligible

📝 Documentation

Strengths

• README examples are clear and practical
• Both syntax options are documented
• The use case (file path → file content) is relatable

Suggestions

1. Docstring for DependentCallable - Could benefit from usage examples
2. Error messages - Consider adding examples to circular dependency errors
3. Type hints in examples - README examples could show type hints for clarity

🧪 Testing

Coverage Analysis

The PR adds tests for:

• ✅ Dependency graph construction with explicit deps
• ✅ Both depends_on and tuple syntax
• ✅ Integration with generation
• ✅ Proper ordering with dependencies

Missing Test Cases

Consider adding tests for:

1. Mixed dependencies - Column with both explicit deps and template-based deps
2. Invalid dependency declarations - Malformed tuple syntax
3. Circular dependencies with explicit deps - Ensure detection still works
4. Async callables with dependencies - Ensure await handling works correctly

Example test:

async def test_mixed_explicit_and_implicit_dependencies(self):
    """Test column with both explicit and template-based dependencies."""
    mock_gen = Mock(spec=GeneratorFunction)
    mock_gen.prompt_template = "Based on {col1}"
    
    schema = {
        "col1": ChoiceSampler(["A"]),
        "col2": ChoiceSampler(["B"]),
        # col3 depends on both col1 (implicit) and col2 (explicit)
        "col3": depends_on(mock_gen, "col2"),
    }
    ds = Dataset(schema, n=2)
    dependencies = ds._build_dependency_graph()
    
    # Should have both dependencies
    assert set(dependencies["col3"]) == {"col1", "col2"}

📊 Overall Assessment

Category	Rating	Notes
Code Quality	⭐⭐⭐⭐⭐	Clean, well-structured, follows existing patterns
Test Coverage	⭐⭐⭐⭐☆	Comprehensive, could add edge case tests
Documentation	⭐⭐⭐⭐☆	Good examples, could expand docstrings
Performance	⭐⭐⭐⭐⭐	No concerns, minimal overhead
Security	⭐⭐⭐⭐⭐	No issues identified
API Design	⭐⭐⭐⭐⭐	Intuitive, consistent, backwards compatible

✅ Recommendation

APPROVE with minor suggestions

This is a solid implementation that adds valuable functionality without breaking existing code. The suggested improvements are minor and mostly preventive. The core implementation is sound and ready to merge.

Before Merging

1. Consider adding validation warning for non-existent dependencies (optional)
2. Add test for mixed explicit/implicit dependencies (optional but recommended)
3. Verify all tests pass with uv run pytest tests/test_dataset_comprehensive.py tests/test_datset.py

Nice to Have (Future)

• More detailed error messages for circular dependencies showing the cycle
• Type stubs or runtime validation for tuple syntax
• Performance benchmarks for large dependency graphs

Great work! This feature will make the library more flexible for complex use cases. 🎉

[claude]

Pull Request Review - PR #29

Summary

This PR adds explicit dependency support for plain callable columns, providing three convenient ways to declare dependencies: call() wrapper with with_ parameter, tuple syntax, and depends_on() alias. The implementation is well-thought-out and includes comprehensive tests.

Code Quality ✅

Strengths

1. Clean API Design: The multiple syntax options (call(), tuple, depends_on()) provide good developer experience
2. Backward Compatibility: The depends_on() function maintains compatibility while call() is the preferred API
3. Comprehensive Testing: 75 new test lines covering all three dependency declaration methods
4. Good Documentation: README examples clearly show usage patterns
5. Proper Separation of Concerns: DependentCallable class cleanly encapsulates the wrapper logic

Code Quality Observations

• Clean implementation of _resolve_column_callable() and _extract_explicit_dependencies()
• Good use of static methods for utility functions
• Stable dependency ordering with duplicate removal (lines 200-204 in dataset.py:200-204)

Potential Issues ⚠️

1. Keyword Argument Handling (dataset.py:350-353)

The call() function extracts with from kwargs but this could be confusing:

with_deps = kwargs.pop("with", None)  # Line 350
if kwargs:
    unexpected = ", ".join(kwargs.keys())
    raise TypeError(f"Unexpected keyword argument(s): {unexpected}")

Issue: While documented in README, users might expect with to work directly as a parameter. The current implementation requires **{"with": [...]} expansion which is verbose.

Recommendation: Consider if this complexity is necessary or if with_ is sufficient. The test on line 247 shows the awkward syntax required.

2. Callable Context Detection Logic (dataset.py:372-388)

The _callable_accepts_context() function has a potential issue:

if not params:
    return False  # No parameters means can't accept context

for param in params:
    if param.kind in (inspect.Parameter.VAR_POSITIONAL, inspect.Parameter.VAR_KEYWORD):
        return True

return True  # Always returns True if has params!

Issue: Lines 384-388 always return True if there are any parameters, even if the callable expects specific non-context arguments. This could cause TypeError at runtime.

Example that would fail:

def needs_two_args(x, y):  # Requires exactly 2 args
    return x + y

call(needs_two_args, with_=["col1"])  # Would call with 1 dict arg, causing TypeError

Recommendation: Check if the signature accepts exactly one positional parameter OR has *args/**kwargs before returning True.

3. Error Handling for Invalid Dependencies

Currently, if a dependency is declared but doesn't exist in the schema, it's silently filtered out (dataset.py:207). This could mask user errors.

Recommendation: Consider logging a warning or raising an error when declared dependencies aren't found in the schema.

4. Type Annotations

The codebase has disallow_untyped_defs = true in mypy config (pyproject.toml:104), but the new functions lack complete type hints:

• _callable_accepts_context return type is declared but parameter could be more specific
• Type hints are present but could be more precise in some cases

Security Considerations ✅

No security concerns identified. The code:

• Doesn't introduce any injection vulnerabilities
• Properly validates input types
• Safely handles user-provided callables

Performance Considerations ✅

• Dependency graph building is O(n) per column, acceptable
• inspect.signature() is called once per callable at initialization, not per-row (good!)
• No performance regressions expected

Test Coverage ✅

Excellent test coverage with 5 new test methods covering:

• call() wrapper with with_ parameter
• call() with with keyword via kwargs expansion
• depends_on() backward compatibility
• Tuple syntax for dependencies
• Dependency graph building with explicit dependencies

Minor suggestion: Add edge case tests for:

• Invalid callable signatures (the context detection issue mentioned above)
• Dependencies that don't exist in schema
• Mixed explicit and template-based dependencies

Documentation ✅

README.md update is clear and provides good examples. The note about with being a Python keyword is helpful.

Minor suggestion: Add a section about when to use explicit dependencies vs template-based dependencies.

Recommendations

Critical (Should Fix)

1. Fix _callable_accepts_context() logic to properly detect when a callable can accept a context dict parameter

High Priority (Should Consider)

2. Add validation for dependencies that reference non-existent columns (warn or error instead of silent filtering)

Nice to Have

3. Simplify keyword argument handling in call() - consider if supporting with via kwargs is worth the complexity
4. Add edge case tests for invalid callable signatures
5. Add type hints to fully satisfy mypy strict mode

Verdict

This is a solid PR that adds valuable functionality with good test coverage. The implementation is clean and well-structured. The critical issue with _callable_accepts_context() should be addressed before merging to prevent runtime TypeErrors. The other recommendations are optional improvements.

Suggested Action: Request changes for the context detection logic, then approve.

---

Review generated by Claude Code

[claude]

Pull Request Review: Support explicit callable column dependencies

Summary

This PR adds support for explicit dependency declaration for plain callables in dataset schemas, complementing the existing automatic dependency detection from generator prompt templates. The implementation is well-designed and thoroughly tested.

Code Quality: ✅ Excellent

Strengths:

1. Clean API Design: Multiple intuitive ways to declare dependencies:

   • call(func, requires=["dep"]) - most explicit
   • call(func, *dependencies) - positional args
   • (callable_fn, ["deps"]) - tuple syntax
   • depends_on(func, "dep") - backwards compatible alias

2. Well-structured code: The DependentCallable wrapper class is clean and maintains separation of concerns

3. Defensive programming: Good error handling for unexpected kwargs in call() (line 354-357)

4. Type safety: Proper type hints throughout the new code

Best Practices: ✅ Strong

1. Backward compatibility: The depends_on() alias ensures existing code won't break
2. Incremental approach: Extends existing dependency graph building rather than replacing it
3. Documentation: Good docstrings and README examples showing usage patterns
4. Testing: Comprehensive test coverage with 6 new test cases covering all scenarios

Potential Issues: ⚠️ Minor Concerns

1. Context Parameter Detection Logic (lines 381-397)

The _callable_accepts_context() function has an issue:

def _callable_accepts_context(func: Callable[..., Any]) -> bool:
    params = list(signature.parameters.values())
    if not params:
        return False  # ❌ No params = can't accept context
    
    for param in params:
        if param.kind in (inspect.Parameter.VAR_POSITIONAL, inspect.Parameter.VAR_KEYWORD):
            return True
    
    return True  # ❌ Always returns True if params exist

Problem: Any function with at least one parameter returns True, even if it has zero-arg requirements like lambda: "value". The loop checking for *args/**kwargs can never reach the final return True unless the loop exits without finding them, but then it still returns True.

Impact: A call(lambda: "value", requires=["dep"]) would incorrectly try to pass context to a no-arg lambda.

Suggested fix:

def _callable_accepts_context(func: Callable[..., Any]) -> bool:
    try:
        signature = inspect.signature(func)
    except (TypeError, ValueError):
        return True  # Conservative default for uninspectable callables
    
    params = list(signature.parameters.values())
    if not params:
        return False
    
    # Check if it can accept a positional argument
    for param in params:
        if param.kind in (
            inspect.Parameter.VAR_POSITIONAL,
            inspect.Parameter.VAR_KEYWORD,
            inspect.Parameter.POSITIONAL_OR_KEYWORD,
            inspect.Parameter.POSITIONAL_ONLY,
        ):
            return True
    
    return False  # KEYWORD_ONLY params only

2. Dependency Deduplication (lines 200-204)

The deduplication preserves order but iterates through the list for each item:

ordered_deps = []
for dep in deps:
    if dep not in ordered_deps:  # O(n) lookup per iteration = O(n²)
        ordered_deps.append(dep)

For small dependency lists this is fine, but could use a set for tracking:

seen = set()
ordered_deps = []
for dep in deps:
    if dep not in seen:
        seen.add(dep)
        ordered_deps.append(dep)

3. Type Validation in _extract_explicit_dependencies() (line 246)

The check isinstance(deps, Iterable) will return True for strings (which are iterable), but strings are already handled above. Consider:

if isinstance(deps, str):
    return [deps]
if isinstance(deps, Iterable) and not isinstance(deps, (str, bytes)):
    return [dep for dep in deps if isinstance(dep, str)]

Performance Considerations: ✅ Good

1. Dependency extraction is done once during graph building (good)
2. The tuple syntax (callable, deps) is unpacked efficiently
3. No performance regression for existing code paths

Security Concerns: ✅ None

No security issues identified. The code doesn't:

• Execute arbitrary strings
• Perform unsafe file operations
• Introduce injection vulnerabilities

Test Coverage: ✅ Excellent

Comprehensive tests covering:

• ✅ call() with requires parameter
• ✅ call() with positional dependencies
• ✅ call() with with alias (via kwargs)
• ✅ Tuple syntax (callable, ["deps"])
• ✅ depends_on() backwards compatibility
• ✅ String vs list handling for requires
• ✅ Dependency graph integration
• ✅ End-to-end generation with explicit dependencies

Missing test cases:

• No-arg callables wrapped with call() (would expose the context detection bug)
• Error cases (e.g., invalid dependency types)
• Interaction between explicit deps and template deps in the same schema

Recommendations

High Priority:

1. Fix _callable_accepts_context() logic to properly detect no-arg callables
2. Add test for call(lambda: "static_value") to prevent regression

Medium Priority:

1. Improve deduplication to O(n) complexity
2. Strengthen type validation in _extract_explicit_dependencies()

Low Priority:

1. Consider adding a test for mixed explicit + template dependencies
2. Add type validation test cases

Overall Assessment: ✅ Approve with Minor Fixes

This is a solid feature addition that follows good design principles. The API is intuitive and the implementation integrates well with existing code. The main concern is the context detection bug which could cause runtime errors with no-arg callables. Once that's fixed, this is ready to merge.

Recommendation: Fix the _callable_accepts_context() function and add corresponding test coverage before merging.

[claude]

Pull Request Review: Support explicit callable column dependencies in dataset schemas

Summary

This PR adds explicit dependency declaration support for plain callable columns, allowing users to specify dependencies via call() wrapper, tuple syntax, or automatic inference from function signatures. The implementation is well-structured and includes comprehensive test coverage.

✅ Strengths

1. Clean API Design

The call() wrapper provides an intuitive interface with multiple ways to declare dependencies:

• requires= parameter (list or string)
• with_= parameter for alternative syntax
• Positional arguments via *dependencies
• Tuple syntax: (callable_fn, ["dep_col"])

2. Backward Compatibility

• Legacy depends_on() function maintained as alias
• Existing callables without explicit deps continue working
• Fallback to context dict for callables without inspect metadata (dataset.py:453)

3. Smart Signature Inference

The _infer_signature_dependencies() method (dataset.py:255) automatically detects dependencies from function parameter names, reducing boilerplate:

def file_content(file_path):  # Automatically infers dependency on file_path column
    return load_chunk(file_path)

4. Comprehensive Test Coverage

130+ lines of new tests covering:

• Explicit dependencies via call() and depends_on()
• Tuple syntax
• Signature inference
• Edge cases (string vs list for requires, with keyword alias)

5. Documentation

README examples clearly demonstrate all three dependency patterns with practical use cases.

---

🔍 Code Quality Issues

1. Type Hints Missing (Critical for codebase with mypy enabled)

The PR introduces several new functions/methods without complete type hints, violating the disallow_untyped_defs = true mypy configuration (pyproject.toml:104):

dataset.py:194-196:

@staticmethod
def _extract_explicit_dependencies(func: Any) -> List[str]:
    """Extract explicit dependency declarations from schema value."""
    deps = []  # Type not specified

dataset.py:377-382:

def call(
    func: Callable[..., Any],
    *dependencies: str,
    requires: Optional[List[str]] = None,
    with_: Optional[List[str]] = None,
    **kwargs,  # kwargs type should be Any
) -> DependentCallable:

Recommendation: Add complete type hints to all new functions to pass mypy checks.

2. Incomplete Iterable Type Check (dataset.py:227-228)

if isinstance(deps, Iterable):
    return [dep for dep in deps if isinstance(dep, str)]

Issue: str is an Iterable, so if deps is a string, this returns a list of individual characters instead of [deps]. This is already handled above (line 224), but the logic is fragile.

Recommendation: Add explicit string check or use collections.abc.Sequence with proper guards.

3. Reserved Keyword Workaround (dataset.py:384)

with_deps = kwargs.pop("with", None)

Issue: Using kwargs to accept Python reserved keyword with is clever but non-standard. The parameter is named with_ but users must pass it as with.

Example:

call(func, **{"with": ["col"]})  # Awkward syntax

Recommendation: Document this clearly in docstring or consider removing with alias in favor of requires.

4. Duplicate Dependency Handling (dataset.py:203-207)

The code removes duplicates but doesn't warn about them:

for dep in deps:
    if dep not in ordered_deps:
        ordered_deps.append(dep)

Recommendation: Consider logging a warning if duplicates are detected, as this might indicate a configuration error.

---

🐛 Potential Bugs

1. Context Dict Collision with Column Names (dataset.py:266-267)

if param.name in CONTEXT_ARG_NAMES:
    continue  # Skip params named 'ctx' or 'context'

Issue: If a column is actually named "ctx" or "context", signature inference will skip it even though it might be a legitimate dependency.

Example:

schema = {
    "ctx": sample.choice(["A", "B"]),  # Column named 'ctx'
    "result": call(lambda ctx: ctx["ctx"])  # Won't infer dependency\!
}

Recommendation: This is an edge case, but worth documenting or handling explicitly.

2. Missing Validation for Circular Dependencies

The PR doesn't add explicit checks for circular dependencies introduced via explicit declarations:

schema = {
    "a": call(lambda ctx: ctx["b"], requires=["b"]),
    "b": call(lambda ctx: ctx["a"], requires=["a"]),  # Circular\!
}

Current behavior: The existing _topological_sort() method should raise a RuntimeError (dataset.py:291), but this should be tested.

Recommendation: Add test case for circular dependency detection with explicit call() dependencies.

---

⚡ Performance Considerations

1. Signature Inspection on Every Build (dataset.py:255)

def _infer_signature_dependencies(self, func: Any, current_column: str) -> List[str]:
    try:
        signature = inspect.signature(target)
    except (TypeError, ValueError):
        return []

Issue: inspect.signature() is called for every column during _build_dependency_graph(), which runs on every dataset initialization.

Recommendation: Consider caching signatures if this becomes a bottleneck for large schemas, though likely premature optimization.

---

🔒 Security Concerns

No significant security issues identified. The code:

• Doesn't execute untrusted code beyond user-provided callables (expected behavior)
• Properly validates dependency types (dataset.py:227-228)
• Uses inspect safely with try-except blocks

---

🧪 Test Coverage Suggestions

Missing Test Cases:

1. Circular dependency detection with explicit call()
2. Mixed explicit + inferred dependencies (e.g., function has both explicit requires= and matching param names)
3. Async callables with call() wrapper
4. Edge case: column named "ctx" or "context"
5. Invalid dependency types (e.g., requires=123)

Test Case Example:

async def test_circular_dependency_detection_explicit():
    """Test that circular deps with call() are caught."""
    schema = {
        "a": call(lambda ctx: ctx["b"], requires=["b"]),
        "b": call(lambda ctx: ctx["a"], requires=["a"]),
    }
    ds = Dataset(schema, n=1)
    
    with pytest.raises(RuntimeError, match="circular dependency|cycle"):
        await ds.generate()

---

📝 Documentation Improvements

README.md

• ✅ Good examples showing all three patterns
• Missing: Explanation of when to use each pattern
• Missing: Performance implications of signature inference

Suggested addition:

#### Choosing a Dependency Pattern
- **Signature inference**: Best for simple functions with clear parameter names
- **Explicit `requires=`**: Use when parameter names don't match column names or for clarity
- **Tuple syntax**: Shorthand for inline lambdas with dependencies

Code Documentation

The call() function docstring should document the with keyword workaround and with_ parameter interaction.

---

🎯 Overall Assessment

Verdict: ✅ Approve with minor changes

This is a well-designed feature with solid implementation. The explicit dependency system addresses a clear need and integrates cleanly with the existing codebase. The main blockers are:

Required Before Merge:

1. ✅ Fix mypy type hint issues (CI will likely fail)
2. ⚠️ Add test for circular dependency detection with call()

Recommended for Follow-up:

• Document edge case behavior ("ctx"/"context" column names)
• Add test for mixed explicit + inferred dependencies
• Consider removing with keyword alias to simplify API

---

Great work on this feature! The signature inference is particularly elegant. 🚀