Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve provider selection logic in Config.get_provider_for method #297

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lenage
Copy link

@lenage lenage commented Oct 9, 2024

Description

This PR enhances the get_provider_for method in the Config class to provide more accurate and efficient provider selection. The updated implementation prioritizes exact matches and then selects the most specific subclass when dealing with inheritance hierarchies.

Changes

  • Modified get_provider_for method to first check for an exact match between the client type and available providers.
  • If no exact match is found, the method now identifies all matching providers based on subclass relationships.
  • Implemented a sorting mechanism to select the most specific (derived) provider when multiple matches are found.

Testing

create a new ProviderClass which is a subclass of OpenAIProvider

class AwesomeProvider(OpenAIProvider):
    def translate_to_provider(self, ell_call: EllCallParams) -> Dict[str, Any]:
         #make some changes

    def translate_from_provider(self,
                                provider_response,
                                ell_call: EllCallParams,
                                provider_call_params: Dict[str, Any],
                                origin_id: str | None = None,
                                logger: Callable[..., None] | None = None) -> Tuple[List[Message] | Dict[str, Any]]:
        return super().translate_from_provider(provider_response, ell_call, provider_call_params, origin_id, logger)

ell.register_provider(AwesomeProvider())

then, if you call ell.simple, it always use OpenAIProvider instead of AwesomeProvider
This PR fix above issue to check inheritance hierarchies

Additional Notes

This change maintains backwards compatibility while providing more precise provider selection, especially in cases involving complex inheritance hierarchies.

@MadcowD
Copy link
Owner

MadcowD commented Oct 11, 2024

I'll take a quick look

@MadcowD
Copy link
Owner

MadcowD commented Oct 12, 2024

Would you be willing to add a few tests for this PR?

@MadcowD
Copy link
Owner

MadcowD commented Oct 12, 2024

Ready to merge just dont want to have regressions in the future

@lenage
Copy link
Author

lenage commented Oct 13, 2024

Would you be willing to add a few tests for this PR?

sure, will add some test soon

@lenage
Copy link
Author

lenage commented Oct 13, 2024

Would you be willing to add a few tests for this PR?

sure, will add some test soon

@MadcowD tests added

@lenage
Copy link
Author

lenage commented Oct 21, 2024

@MadcowD have a sec to check this out?

@MadcowD
Copy link
Owner

MadcowD commented Oct 21, 2024

Can you add an assertion error or a Warnung if there are two mstching providers of the same mro lehnte

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