-
Notifications
You must be signed in to change notification settings - Fork 21
Fix OpenAI config bug #54
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request fixes a bug in the OpenAI and ChatGPT configuration by correcting the assignment of the frequency_penalty field. It also adds tests to ensure that the load_config function preserves all configuration values.
- Correct frequency_penalty assignment in neural_providers/openai.py and neural_providers/chatgpt.py.
- Added test cases in test/python/test_openai.py and test/python/test_chatgpt.py to validate configuration values.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
test/python/test_openai.py | Added test_load_config_valid_values to verify config mapping. |
test/python/test_chatgpt.py | Added test_load_config_valid_values to verify config mapping. |
neural_providers/openai.py | Fixed frequency_penalty assignment to use the correct field. |
neural_providers/chatgpt.py | Fixed frequency_penalty assignment to use the correct field. |
Comments suppressed due to low confidence (2)
neural_providers/openai.py:144
- The frequency_penalty field was incorrectly set to presence_penalty; the change correctly assigns frequency_penalty.
- frequency_penalty=presence_penalty,
neural_providers/chatgpt.py:154
- The frequency_penalty field was incorrectly set to presence_penalty; the change correctly assigns frequency_penalty.
- frequency_penalty=presence_penalty,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a typo in mapping frequency_penalty
when building provider configs and adds tests to verify that load_config
preserves all supplied values for both OpenAI and ChatGPT.
- Correct
frequency_penalty
assignment inneural_providers/openai.py
andneural_providers/chatgpt.py
- Add
test_load_config_valid_values
in bothtest_openai.py
andtest_chatgpt.py
to verify all fields are loaded
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
test/python/test_openai.py | Add test_load_config_valid_values for OpenAI provider |
test/python/test_chatgpt.py | Add test_load_config_valid_values for ChatGPT provider |
neural_providers/openai.py | Fix frequency_penalty to use correct variable |
neural_providers/chatgpt.py | Fix frequency_penalty to use correct variable |
Comments suppressed due to low confidence (2)
test/python/test_openai.py:69
- Consider adding tests for default fallback values (e.g., omitting
presence_penalty
orfrequency_penalty
) to ensureload_config
applies its default settings correctly.
def test_load_config_valid_values():
test/python/test_chatgpt.py:84
- [nitpick] The mock variable name
compl_mock
is inconsistent withcompletion_mock
used elsewhere; consider renaming tocompletion_mock
for clarity and consistency.
mock.patch.object(chatgpt, 'get_chatgpt_completion') as compl_mock:
Summary
frequency_penalty
field when building config objectsload_config
preserves all values for OpenAI and ChatGPT providersTesting
python -m pytest -q