Skip to content

feat: added Grok as a provider and related functionality#45

Merged
johnny-emp merged 13 commits intomainfrom
feat/add-grok-provider
Apr 16, 2025
Merged

feat: added Grok as a provider and related functionality#45
johnny-emp merged 13 commits intomainfrom
feat/add-grok-provider

Conversation

@ahmedhamedaly
Copy link
Collaborator

  • Updated imports to include GrokProvider in init.py files.
  • Added to_grok method in GenericTool class for converting to Grok tool format.
  • Included GrokModelType in the providers' all list.

- Updated imports to include GrokProvider in __init__.py files.
- Added to_grok method in GenericTool class for converting to Grok tool format.
- Included GrokModelType in the providers' __all__ list.
@johnny-emp
Copy link
Contributor

90% of your code is copied and pasted, please try to use class inheritance

from .types import GrokModelType


class Request(BaseModel):
Copy link
Contributor

@johnny-emp johnny-emp Apr 11, 2025

Choose a reason for hiding this comment

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

this looks like it can mostly inherit from OpenAI, is there a reason to not inherit from the OpenAI implementation?

Copy link
Contributor

Choose a reason for hiding this comment

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

It also might be better to make a generic type that takes an argument for the ModelType, ie.

ModelType = TypeVar("ModelType")
class StandardRequest(Generic[ModelType]):
     model: ModelType

and then it can be used by all platforms that mimic the openai format

@johnny-emp
Copy link
Contributor

I made a small PR to this branch fixing a couple type issues, this should help by simplifying the tools type: #46

),
)

def to_grok(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

we actually don't need this anymore, since we inverted controls to Providers


tools: Annotated[
Optional[list[GenericTool]],
PlainSerializer(
Copy link
Contributor

Choose a reason for hiding this comment

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

same with this, I believe we can remove the PlainSerializer. I was trying to use Annotations for the serialization initially, but it became simpler to just define the Tool format per provider. Basically we switched to utilize IOC https://en.wikipedia.org/wiki/Inversion_of_control but there's still some relics of the earlier solution.

@johnny-emp
Copy link
Contributor

a couple small comments, but this looks great now. Thanks for the hard work!

@johnny-emp johnny-emp force-pushed the feat/add-grok-provider branch from 246fd2a to 4c8b488 Compare April 15, 2025 20:58
@johnny-emp johnny-emp merged commit 0937b2b into main Apr 16, 2025
5 checks passed
@johnny-emp johnny-emp deleted the feat/add-grok-provider branch April 16, 2025 04:09
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