Skip to content

Refactor: Centralize model settings and add validation tests#119

Merged
seratch merged 1 commit intoseratch:mainfrom
PeterDaveHello:refactor
Aug 17, 2025
Merged

Refactor: Centralize model settings and add validation tests#119
seratch merged 1 commit intoseratch:mainfrom
PeterDaveHello:refactor

Conversation

@PeterDaveHello
Copy link
Contributor

Decouple model-specific configurations from application logic to improve maintainability and extensibility.

  • Centralize Settings:

    • Moved hardcoded context lengths from openai_ops.py into a new MODEL_CONTEXT_LENGTHS dictionary in openai_constants.py.
    • Extracted the default token calculation model into a new DEFAULT_TOKEN_COUNT_MODEL constant.
  • Unify Alias Resolution:

    • Created a single resolve_model_alias() helper function in openai_constants.py to handle alias chains and prevent circular dependencies.
    • Refactored context_length() and calculate_num_tokens() to use this shared function, removing duplicated logic.
  • Add Validation Tests:

    • Introduced a new test suite (tests/model_constants_test.py) to verify the refactoring.
    • Tests cover alias resolution, error handling for unregistered models, circular dependency detection, and configuration completeness for all model aliases.

Change summary from GitHub Copilot:

This pull request refactors how model aliases and context lengths are managed for OpenAI models, making the codebase more maintainable and robust. It centralizes model alias resolution, simplifies context length and token counting logic, and adds comprehensive tests for these mechanisms.

Refactoring and Centralization of Model Metadata:

  • Added a new MODEL_CONTEXT_LENGTHS mapping and a resolve_model_alias function to openai_constants.py, which standardizes how model aliases are resolved and context lengths are looked up. This removes the need for hardcoded if/else chains throughout the codebase.
  • Introduced DEFAULT_TOKEN_COUNT_MODEL to provide a fallback model when none is specified for token counting.

Simplification of Token Counting and Context Length Logic:

  • Updated calculate_num_tokens and context_length in openai_ops.py to use the new alias resolution and context length mapping, replacing the previous verbose conditional logic with a more maintainable approach. [1] [2]
  • Ensured that token counting in messages_within_context_window now always uses the correct model from the context, improving accuracy for different model types.

Testing and Validation:

  • Added a new test suite (model_constants_test.py) to verify alias resolution, error handling for unregistered models, detection of circular dependencies, and coverage of all model aliases.
  • Added a test in openai_ops_test.py to ensure that the correct model is passed to token counting during message window calculations.

Code Cleanup:

  • Removed unused model constants from openai_ops.py imports, relying on centralized constants and mappings instead.

These changes make the codebase more scalable as new models are added and improve reliability by consolidating model-related logic and increasing test coverage.

Decouple model-specific configurations from application logic to improve
maintainability and extensibility.

- Centralize Settings:
  - Moved hardcoded context lengths from `openai_ops.py` into a new
    `MODEL_CONTEXT_LENGTHS` dictionary in `openai_constants.py`.
  - Extracted the default token calculation model into a new
    `DEFAULT_TOKEN_COUNT_MODEL` constant.

- Unify Alias Resolution:
  - Created a single `resolve_model_alias()` helper function in
    `openai_constants.py` to handle alias chains and prevent circular
    dependencies.
  - Refactored `context_length()` and `calculate_num_tokens()` to use
    this shared function, removing duplicated logic.

- Add Validation Tests:
  - Introduced a new test suite (`tests/model_constants_test.py`) to
    verify the refactoring.
  - Tests cover alias resolution, error handling for unregistered
    models, circular dependency detection, and configuration
    completeness for all model aliases.
@seratch seratch merged commit 47323b4 into seratch:main Aug 17, 2025
5 checks passed
@PeterDaveHello PeterDaveHello deleted the refactor branch August 17, 2025 06:08
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