Skip to content

fix: better handling of extra model params in Harness#1183

Merged
chakravarthik27 merged 8 commits intorelease/2.6.0from
fix/issues-found-while-testing-bug-fixes-in-260-rc-version
Mar 7, 2025
Merged

fix: better handling of extra model params in Harness#1183
chakravarthik27 merged 8 commits intorelease/2.6.0from
fix/issues-found-while-testing-bug-fixes-in-260-rc-version

Conversation

@chakravarthik27
Copy link
Collaborator

This pull request includes changes to the __init__ method in the langtest/langtest.py file to enhance the handling of additional model information. The most important changes include restructuring the way additional model information is merged and ensuring that model parameters from the configuration are properly integrated.

Enhancements to model information handling:

  • langtest/langtest.py: Removed the old approach of merging additional model information and replaced it with a more structured way of handling additional info for each model. This includes filtering out specific keys and merging configuration parameters conditionally.

Copy link
Contributor

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@chakravarthik27 chakravarthik27 requested a review from Copilot March 4, 2025 12:19
Copy link
Contributor

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.

PR Overview

This PR enhances the handling of additional model parameters in the Harness by restructuring how extra model information is merged from both the configuration and per-model inputs.

  • Restructures the merging of additional model info for models provided as a list.
  • Consolidates the handling of model parameters from the configuration in both list and non-list branches.

Reviewed Changes

File Description
langtest/langtest.py Refactors the merging logic of additional model info in the init method

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

Comments suppressed due to low confidence (1)

langtest/langtest.py:221

  • [nitpick] Consider renaming the loop variable 'i' to a more descriptive name (e.g., 'model_entry') and avoid reassigning the 'model' variable inside the loop to improve clarity.
for i in model:

@chakravarthik27 chakravarthik27 requested a review from Copilot March 4, 2025 12:32
Copy link
Contributor

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.

PR Overview

This PR improves the handling of additional model parameters in the Harness by restructuring how extra model information is merged and integrated.

  • Introduces model-specific additional info extraction within a loop when multiple models are provided.
  • Merges configuration parameters conditionally for each model based on its presence in the configuration.
  • Maintains legacy behavior for single model scenarios while enhancing multi-model support.

Reviewed Changes

File Description
langtest/langtest.py Revised init method to restructure merging of additional model information and handle multiple models

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

Comments suppressed due to low confidence (2)

langtest/langtest.py:246

  • The assignment to 'self.model' is done within the loop, causing repeated assignment on each iteration. Consider moving this assignment outside the loop to ensure it is set only once after processing all models.
self.model = model_dict

langtest/langtest.py:223

  • [nitpick] Reassigning the 'model' variable within the loop can be confusing as it shadows the outer 'model' parameter. Consider using a different variable name to clarify the distinction between the loop item and the outer model.
model = i["model"]

@chakravarthik27 chakravarthik27 merged commit 817b917 into release/2.6.0 Mar 7, 2025
3 checks passed
@chakravarthik27 chakravarthik27 deleted the fix/issues-found-while-testing-bug-fixes-in-260-rc-version branch March 12, 2025 06:06
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.

3 participants