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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hf inference endpoint #18

Merged
merged 9 commits into from
Jul 28, 2023
Merged

Add hf inference endpoint #18

merged 9 commits into from
Jul 28, 2023

Conversation

Wirg
Copy link
Collaborator

@Wirg Wirg commented Jul 27, 2023

No description provided.

@github-actions
Copy link

github-actions bot commented Jul 27, 2023

LOGAF Level 3 - /home/runner/work/GenossGPT/GenossGPT/demo/constants/model_configs.py

This code is generally good, but there are areas for potential improvement. The configure_open_ai_module method directly modifies the openai module's attributes, which could lead to unexpected behavior if the openai module is used elsewhere in the codebase. Consider using a context manager or another approach to ensure that these changes are only in effect for the duration of the relevant operation.

Example:

from contextlib import contextmanager

@contextmanager
def configured_openai(api_key, endpoint_url):
    original_api_key = openai.api_key
    original_api_base = openai.api_base

    openai.api_key = api_key
    openai.api_base = endpoint_url

    try:
        yield
    finally:
        openai.api_key = original_api_key
        openai.api_base = original_api_base

LOGAF Level 2 - /home/runner/work/GenossGPT/GenossGPT/genoss/api/embeddings_routes.py

This code functions, but has significant issues needing attention. The post_embeddings function only supports the "gpt4all" model. If other models are to be supported in the future, this function will need to be updated. Consider refactoring this function to be more flexible and support multiple models.

Example:

async def post_embeddings(model: str, input: str) -> list[float]:
    if model not in SUPPORTED_MODELS:
        raise NotImplementedError(f"Model {model} is not supported.")

    gpt = SUPPORTED_MODELS[model]()
    response = gpt.generate_embedding(input)
    return response

LOGAF Level 3 - /home/runner/work/GenossGPT/GenossGPT/genoss/entities/chat/chat_completion.py

This code is generally good, but there are areas for potential improvement. The nested classes Choice and Usage could be defined outside of ChatCompletion for better readability and reusability. Also, consider using Pydantic models for these classes to leverage its features like data validation, serialization, and documentation.

LOGAF Level 2 - /home/runner/work/GenossGPT/GenossGPT/genoss/llm/hf_hub/base_hf_hub.py

This code functions, but has significant issues needing attention. The generate_embedding method raises a NotImplementedError, which could be confusing for future developers. If this method is not applicable for this class, consider revising the class hierarchy or interfaces to avoid having to implement it.

LOGAF Level 2 - /home/runner/work/GenossGPT/GenossGPT/genoss/services/model_factory.py

This code functions, but has significant issues needing attention. The get_model_from_name method has a lot of if statements, which could be hard to maintain as more models are added. Consider using a dictionary or a design pattern like Factory to map model names to their corresponding classes.

Example:

MODEL_MAPPING = {
    "gpt-4": OpenAILLM,
    "gpt-3.5-turbo": OpenAILLM,
    "gpt4all": Gpt4AllLLM,
    # ...
}

def get_model_from_name(name: str, api_key: str | None = None) -> BaseGenossLLM | None:
    model_class = MODEL_MAPPING.get(name.lower())
    if model_class is None:
        return None
    return model_class(api_key=api_key)

馃攧馃敡馃摎


Powered by Code Review GPT

@Wirg Wirg marked this pull request as ready for review July 27, 2023 16:42
@StanGirard StanGirard merged commit fd60f05 into main Jul 28, 2023
7 checks passed
@StanGirard StanGirard deleted the add-hf-inference-endpoint branch July 28, 2023 07:16
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.

None yet

2 participants