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

Fix: Correct API key loading from environment variables (closes #10) #24

Open
wants to merge 2 commits into
base: master-backup
Choose a base branch
from

Conversation

Theigrams
Copy link

@Theigrams Theigrams commented Aug 26, 2024

Description

This pull request addresses an issue where the llm_params.api_key and text_embedding.llm.api_key variables were being incorrectly set to the literal string "<API_KEY>" due to a lack of proper validation after reading the environment variable GRAPHRAG_API_KEY. The issue was identified in lines 56 and 58 of the code. The proposed fix includes adding conditional checks to ensure that if the API keys are set to "<API_KEY>", they are replaced with the correct values from the respective configuration files (llm_config['api_key'] and embeddings_config['llm']['api_key']).

Related Issues

This pull request addresses issue #10.

Proposed Changes

  • Added a conditional check for llm_params.api_key to replace "<API_KEY>" with the correct value from llm_config['api_key'].
  • Added a conditional check for text_embedding.llm.api_key to replace "<API_KEY>" with the correct value from embeddings_config['llm']['api_key'].
  • Added a check to verify if llm_params.api_key or text_embedding.llm.api_key is empty after loading. If either is empty, a ValueError is raised to notify the user.

Checklist

  • I have tested these changes locally.
  • I have reviewed the code changes.
  • I have updated the documentation (if necessary).
  • I have added appropriate unit tests (if applicable).

Additional Notes

No additional notes at this time.

@KylinMountain
Copy link
Owner

Would you mind to add a further check if api_key is empty, then raise an Exception?

@Theigrams
Copy link
Author

Would you mind to add a further check if api_key is empty, then raise an Exception?

Thanks for the suggestion! I've added checks to raise a ValueError if the API keys are empty.
Let me know if anything else needs adjustment.

@KylinMountain
Copy link
Owner

Don't know is it a good idea to raise exception since lots of people use local llm, what's your opinion?

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