Skip to content

feat: Enhance config validation (Resolves #33) - #42

Open
dyrpsf wants to merge 2 commits into
draeger-lab:devfrom
dyrpsf:feature/issue-33-config-validation
Open

feat: Enhance config validation (Resolves #33)#42
dyrpsf wants to merge 2 commits into
draeger-lab:devfrom
dyrpsf:feature/issue-33-config-validation

Conversation

@dyrpsf

@dyrpsf dyrpsf commented Jul 13, 2026

Copy link
Copy Markdown

Description

This PR introduces a robust, dictionary-based configuration validation system to resolve Issue #33. It centralizes the validation logic into a new config_validator.py utility, replacing the previously verbose YAML traversal[cite: 2].

Key Changes

  • Centralized Schema Validation: Created SchemaField and validate_config to allow developers to define expected configuration structures cleanly.
  • Deprecated Keyword Handling:
    • Replaced the USER -> None fallback with a system that warns the user and automatically applies sensible developer-defined defaults[cite: 2].
    • The _USER_ keyword now throws an explicit ConfigValidationError for missing required parameters[cite: 2].
  • Input Correctness: Added built-in support to check if numerical inputs are within specific ranges (min_val/max_val) and if string parameters match an expected list (allowed_values)[cite: 2].
  • Path Verification: Added is_path and path_must_exist flags to strictly verify that provided file/directory paths actually exist on the system[cite: 2].

Testing

Tested locally to ensure accurate error throwing for missing parameters and out-of-bound variables, as well as successful fallbacks for deprecated keywords.

Resolves draeger-lab#33. Replaces verbose YAML traversal and USER/_USER_ keyword logic with a recursive dictionary schema validator. Adds support for type enforcement, range checking, path validation, and sensible defaults.
@dyrpsf
dyrpsf changed the base branch from main to dev July 16, 2026 11:29
@draeger
draeger requested review from cb-Hades and Copilot July 29, 2026 11:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a schema-driven, dictionary-based configuration validation utility intended to centralize and simplify config validation logic, in line with Issue #33.

Changes:

  • Added src/specimen/util/config_validator.py introducing SchemaField, validate_config, and ConfigValidationError.
  • Updated src/specimen/util/__init__.py to expose the new validation utilities at the specimen.util package level.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
src/specimen/util/config_validator.py New schema-based config validation implementation with type/constraint/path checks and legacy keyword handling.
src/specimen/util/init.py Re-exports new validator symbols from the util package namespace.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/specimen/util/config_validator.py Outdated
Comment thread src/specimen/util/config_validator.py
Comment thread src/specimen/util/config_validator.py Outdated
Comment thread src/specimen/util/__init__.py Outdated
Fixes nested USER missing value check logic, cleans up type error formatting, removes unused Callable import, and resolves export ambiguity in __init__.py.
@dyrpsf

dyrpsf commented Aug 1, 2026

Copy link
Copy Markdown
Author

Thanks for the thorough review! I've pushed updates to address all the feedback:

  • Fixed Keyword Logic: Refactored the required field checks so legacy USER keywords are correctly caught alongside _USER_ and handled appropriately without nested logic traps.
  • Cleaner Type Errors: Updated the type validation error message to use type(value).__name__ for readability.
  • Cleaned Imports: Removed the unused Callable import to ensure linting passes.
  • Resolved API Ambiguity: Updated __init__.py to export the config_validator module directly rather than individual symbols.

Let me know if anything else is needed!

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/specimen/util/config_validator.py:97

  • This “Clean up legacy 'USER' keyword” block is unreachable because the earlier if value is None or value == "_USER_" or value == "USER": ... continue already handles all value == "USER" cases. Removing this dead code will simplify control flow and prevent confusion when modifying the placeholder logic later.
        # Clean up legacy 'USER' keyword
        if value == "USER":
            logger.warning(
                f"Deprecated 'USER' keyword found at '{current_path}'. Using default: {rule.default}"
            )
            value = rule.default

src/specimen/util/config_validator.py:105

  • bool is a subclass of int in Python, so this numeric bounds check will also run for boolean values (and expected_type=int would accept booleans). Excluding bool here avoids surprising validation behavior when the schema contains integer fields and the YAML provides true/false.
        if isinstance(value, (int, float)):

src/specimen/util/config_validator.py:90

  • The legacy placeholder for “required user input” in this repo is __USER__ (see specimen/util/set_up.py:309 and the example YAMLs), but this validator only checks for _USER_. That means __USER__ would pass through as a normal string and likely reach downstream code. Also, when value == "_USER_" and the field is required, the code raises before the deprecation log/specific message, so users won’t see the intended guidance. Consider normalizing both __USER__ and _USER_ up front and always raising a consistent error for them.

This issue also appears in the following locations of the same file:

  • line 91
  • line 105
        # 1. Check Required fields (Replaces _USER_ and USER logic)
        if value is None or value == "_USER_" or value == "USER":
            if rule.required:
                raise ConfigValidationError(
                    f"Missing required configuration parameter: '{current_path}'"

src/specimen/util/init.py:3

  • The PR description states that this PR “replac[es] the previously verbose YAML traversal” for config validation, but the current entrypoints still call specimen.util.set_up.validate_config (e.g. specimen/hqtb/workflow.py:49, specimen/cmpb/workflow.py:48-49). Exporting config_validator from specimen.util doesn’t switch any existing code over, so Issue #33 isn’t fully resolved by these changes alone.
__all__ = ["util", "set_up", "config_validator"]

from . import util, set_up, config_validator

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.

2 participants