Skip to content

Conversation

markurtz
Copy link
Collaborator

@markurtz markurtz commented Sep 4, 2025

Summary

Refactors base configuration and model classes to improve loading capabilities, standardize APIs, and enhance type safety. This PR modernizes the codebase with better type annotations, streamlined loading methods, and improved documentation while maintaining backward compatibility.

Details

  • Enhanced VerifierConfig loading:
    • Renamed from_config() to from_pretrained() for consistency with transformers
    • Added support for loading from multiple sources (PreTrainedModel, PretrainedConfig, paths)
    • Improved parameter resolution and error handling
  • Modernized type annotations:
    • Migrated from Union types to Python 3.10+ union syntax (str | int)
    • Added from __future__ import annotations for Python 3.9 support
    • Improved type hints throughout config and model classes
  • Standardized documentation:
    • Enhanced docstrings with consistent formatting and comprehensive parameter descriptions
    • Improved class-level documentation with better examples and usage patterns
    • Added clear inheritance and compatibility information
  • Improved model loading and attachment:
    • Enhanced verifier attachment with add_to_config parameter for better config management
    • Improved error messages and validation in model initialization
    • Streamlined state dictionary handling to exclude verifier parameters
  • Code quality improvements:
    • Added TC003 to ruff ignore list to allow typing imports outside type checking blocks
    • Consistent code formatting and structure across modules
    • Better separation of concerns in class methods
  • Updated tests:
    • Reorganized test structure with comprehensive class-based organization
    • Enhanced test coverage with parameterized fixtures and edge case testing
    • Improved test naming and documentation for better maintainability

Test Plan

  • Expanded and added unit tests to for the full functionality

Related Issues

  • N/A

@markurtz markurtz self-assigned this Sep 4, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

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 refactors base configuration and model classes to improve loading capabilities, standardize APIs, and enhance type safety. The update modernizes the codebase by adopting Python 3.10+ union syntax, implementing better loading methods, and organizing test suites into comprehensive class-based structures.

  • Enhanced VerifierConfig loading by renaming from_config() to from_pretrained() for consistency with transformers
  • Modernized type annotations by migrating from Union types to Python 3.10+ union syntax (str | int)
  • Reorganized test structure with comprehensive class-based organization and improved coverage

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/unit/test_model.py Comprehensive test reorganization with class-based structure and enhanced test coverage
tests/unit/test_config.py Complete test suite restructuring with parameterized fixtures and class organization
tests/integration/test_config.py Updated method call from from_config to from_pretrained
src/speculators/models/eagle.py Added add_to_config parameter to attach_verifier method
src/speculators/model.py Enhanced type annotations and improved documentation with union syntax migration
src/speculators/config.py Renamed method and improved type annotations with comprehensive documentation updates
pyproject.toml Added TC003 to ruff ignore list for typing imports

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

github-actions bot commented Sep 4, 2025

📦 Build Artifacts Available
The build artifacts (`.whl` and `.tar.gz`) have been successfully generated and are available for download: https://github.com/vllm-project/speculators/actions/runs/17595470840/artifacts/3969102648.
They will be retained for up to 30 days.
Commit: 401a92d

Copy link
Collaborator

@rahul-tuli rahul-tuli left a comment

Choose a reason for hiding this comment

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

A few nits but looks good overall!

Copy link
Collaborator

@shanjiaz shanjiaz left a comment

Choose a reason for hiding this comment

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

Looks good! Can we try to resolve some of the failed tests as well?

Copy link
Collaborator

@dsikka dsikka left a comment

Choose a reason for hiding this comment

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

Question about Python 3.9 which we can also just update to say we do not support as it is coming near EOL

Should this branch not point to main?
How should we be landing it?

@dsikka dsikka requested a review from fynnsu September 8, 2025 19:59
Copy link
Collaborator

@fynnsu fynnsu left a comment

Choose a reason for hiding this comment

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

Looks good! Left a couple comments below.

@markurtz markurtz force-pushed the features/converters/transformers-utils branch 5 times, most recently from 50eb873 to a3c0f80 Compare September 9, 2025 16:00
@markurtz markurtz force-pushed the features/converters/config-updates branch from f1bab30 to a41d2eb Compare September 9, 2025 17:17
@markurtz markurtz force-pushed the features/converters/config-updates branch from 05dab34 to 491e702 Compare September 9, 2025 18:26
Signed-off-by: Mark Kurtz <[email protected]>
Copy link
Collaborator

@fynnsu fynnsu left a comment

Choose a reason for hiding this comment

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

Thanks Mark!

Signed-off-by: Mark Kurtz <[email protected]>
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.

5 participants