[Core] Throw an error when the OAI_CONFIG_LIST is missing. #1082
Conversation
…CONFIG_LIST, and it is missing.
afourney
requested review from
sonichi,
qingyun-wu,
rickyloynd-microsoft,
yiranwu0 and
a team
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1082 +/- ##
===========================================
+ Coverage 30.81% 60.89% +30.08%
===========================================
Files 30 30
Lines 4037 4033 -4
Branches 915 964 +49
===========================================
+ Hits 1244 2456 +1212
+ Misses 2714 1322 -1392
- Partials 79 255 +176
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
All those errors are also from misconfigured tests.... they should have been detected long ago, but were masked by this very issue I am correcting here. |
…not try to load the config_list when skipping OAI tests.
Ok, I think I fixed the merge conflict. Please take a look. |
|
Sorry for another conflict. Hopefully not difficult to resolve. |
Ok, that one was easier :) |
* update openai model support * new gpt3.5 * docstr * function_call and content may co-exist * test function call --------- Co-authored-by: Qingyun Wu <[email protected]>
…#1082) * Throw an explicit and proper error when someone asks to load the OAI_CONFIG_LIST, and it is missing. * Updated to use pytest.raises. Added docstring. Updated some tests to not try to load the config_list when skipping OAI tests. * Fixed wrong indentation in config_list_from_json, and updated test_utils to work with non-empty lists. * Read key location from global constants. * Added missingpath. * Moved config_list_from_json to inside a skip check.
Why are these changes needed?
We silently return an empty list when someone explicitly asks for an OAI_CONFIG_LIST and it can't be found. Paired with other default behavior (e.g, #1077) this can lead to costly calls to GPT-4.
Tackling this was actually one of the very first commits I pushed to autogen in #174. At the time, I was advised that changing default behavior was unwise and that I should perhaps just output an warning message.
Given #1077, and countless other issue since (maybe, #1025), I think it's about time that I go with my original instinct, and correct this unexpected and potentially dangerous behavior.
Related issue number
#1077, #1076, #1025, #174, #178, and so so many other.
Checks