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

Some Fix & code quality improvements #22

Merged
merged 7 commits into from
Jul 31, 2023
Merged

Conversation

Wirg
Copy link
Collaborator

@Wirg Wirg commented Jul 31, 2023

  • fix commented code and a bug in demo
  • use pydantic model and return the class
  • refactor to share code between llms

@github-actions
Copy link

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

1. Abstract methods: The generate_embedding method is marked as abstract but it has an implementation. If this method should have a default implementation, it should not be marked as abstract. If subclasses are expected to provide their own implementation, the method should raise NotImplementedError.

Example:

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

2. Method naming: The method _chat_completion_from_langchain_llm starts with an underscore, which typically indicates a private method. If this method is intended to be used outside of this class, consider removing the underscore.


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

1. Use of lower() method: The name.lower() method is called multiple times. Consider storing the result in a variable to improve performance.

Example:

lower_name = name.lower()
if lower_name in OPENAI_NAME_LIST:
    ...
if lower_name == "gpt4all":
    ...

2. Use of startswith() method: The startswith() method is used to check if the name starts with certain strings. This could lead to false positives if a model name starts with the same characters but is not the intended model. Consider using a more precise method of matching model names.


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

1. Abstract base class: The class HuggingFaceInferenceEndpointLLM is marked as an abstract base class (ABC), but it doesn't have any abstract methods. If there are methods that should be implemented by subclasses, mark them as abstract using the @abstractmethod decorator.

2. Error handling: The generate_embedding method raises a NotImplementedError with a message that it's not used for the Hugging Face Inference API. If this method should not be called, consider removing it from the class. If it's required for some subclasses but not others, the error message should explain this.


🔍📚🔧


Powered by Code Review GPT

@StanGirard StanGirard merged commit 5c33854 into main Jul 31, 2023
7 checks passed
@StanGirard StanGirard deleted the some-code-improvements branch July 31, 2023 15:48
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