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

feat: hugging face in demo #15

Merged
merged 2 commits into from
Jul 23, 2023
Merged

feat: hugging face in demo #15

merged 2 commits into from
Jul 23, 2023

Conversation

mattzcarey
Copy link
Contributor

No description provided.

@github-actions
Copy link

github-actions bot commented Jul 21, 2023

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

The code is generally good, but there are areas for potential improvement.

  1. The code could benefit from some refactoring to improve readability and maintainability. For instance, the code block that handles the API key could be extracted into a separate function.

  2. The error handling could be improved. Currently, the code catches all exceptions and simply prints the error message. It would be better to catch specific exceptions and handle them appropriately.

Here's an example of how you could refactor the API key handling:

def get_api_key(user_api_key, model_name, huggingface_api_key, openai_api_key):
    if user_api_key:
        return user_api_key
    elif model_name.startswith("hf"):
        return huggingface_api_key
    else:
        return openai_api_key

api_key = get_api_key(api_key, model_name, huggingface_api_key, openai_api_key)
if not api_key:
    st.error("Please provide an API key")
    st.stop()

LOGAF Level 3 - /home/runner/work/GenossGPT/GenossGPT/genoss/llm/base_genoss.py

The code is generally good, but there are areas for potential improvement.

  1. The Not import from ast is not used and should be removed.

  2. The generate_answer and generate_embedding methods should raise a NotImplementedError instead of returning it.

Here's an example of how you could improve the generate_answer and generate_embedding methods:

@abstractmethod
def generate_answer(self, prompt: str) -> Dict:
    raise NotImplementedError

@abstractmethod
def generate_embedding(self, text: str) -> List[float]:
    raise NotImplementedError

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

The code functions, but has significant issues needing attention.

  1. The generate_embedding method raises a NotImplementedError with a message saying it's not used for Hugging Face Inference API. If this method is not used, it should not be part of this class.

  2. The __init__ method raises an HTTPException if the API key is missing. This is not appropriate for a class constructor. It would be better to raise a ValueError or a custom exception.

Here's an example of how you could improve the __init__ method:

def __init__(self, api_key, *args, **kwargs):
    super().__init__(*args, **kwargs)

    if api_key is None:
        raise ValueError("API key missing")

    self.huggingfacehub_api_token = api_key

LOGAF Level 3 - /home/runner/work/GenossGPT/GenossGPT/genoss/llm/local/gpt4all.py

The code is generally good, but there are areas for potential improvement.

  1. The generate_embedding method takes a parameter embedding which is either a string or a list of strings. The method then joins the list into a string. This is confusing and could lead to bugs. It would be better to have separate methods for different types of input.

Here's an example of how you could improve the generate_embedding method:

def generate_embedding_from_string(self, text: str):
    gpt4all_embd = GPT4AllEmbeddings()
    return gpt4all_embd.embed_query(text)

def generate_embedding_from_list(self, text_list: list[str]):
    gpt4all_embd = GPT4AllEmbeddings()
    return gpt4all_embd.embed_query(" ".join(text_list))

LOGAF Level 3 - /home/runner/work/GenossGPT/GenossGPT/genoss/llm/openai/openai_llm.py

The code is generally good, but there are areas for potential improvement.

  1. The __init__ method raises a ValueError if the API key is missing. This is good, but it would be better to provide a more specific error message.

  2. The generate_embedding method creates a new instance of OpenAIEmbeddings every time it's called. This could be inefficient if the method is called frequently. It would be better to create the instance once and reuse it.

Here's an example of how you could improve the __init__ and generate_embedding methods:

def __init__(self, model_name: str, api_key, *args, **kwargs):
    super().__init__(*args, **kwargs)

    if api_key is None:
        raise ValueError("API key is missing")

    self.openai_api_key = api_key
    self.model_name = model_name
    self.embedding_model = OpenAIEmbeddings()

def generate_embedding(self, text: str):
    return self.embedding_model.embed_query(text)

馃敡馃攳馃憤


Powered by Code Review GPT

@StanGirard StanGirard merged commit 0a908b6 into main Jul 23, 2023
2 checks passed
@StanGirard StanGirard deleted the feat/demo-for-hf-gpt2 branch July 23, 2023 19:17
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