-
Notifications
You must be signed in to change notification settings - Fork 4.9k
kFix: Stop after all primary vendors in multi-vendor config #290
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?
kFix: Stop after all primary vendors in multi-vendor config #290
Conversation
Summary of ChangesHello @Aitous, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where, in multi-vendor configurations, the system would continue to iterate through fallback vendors even after all primary vendors had been attempted. The changes introduce a more granular tracking mechanism for primary vendors and a new stopping condition that ensures the process terminates once all primary vendors have been tried. This refinement prevents redundant API calls and establishes more predictable behavior for data routing in environments with multiple primary vendors, without altering the established single-vendor stopping logic. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request aims to correct the stopping logic for multi-vendor configurations, ensuring the system stops after all primary vendors are attempted. The implementation correctly introduces a list to track attempted primary vendors. However, there's a logical flaw where the stopping condition for multiple vendors is only checked upon a successful API call, which contradicts the goal of stopping even if some primary vendors fail. I've suggested a correction to move this logic. I also noted a potential edge case with duplicate vendors in the configuration that could cause premature termination.
| if vendor_results: | ||
| results.extend(vendor_results) | ||
| successful_vendor = vendor | ||
| result_summary = f"Got {len(vendor_results)} result(s)" | ||
| print(f"SUCCESS: Vendor '{vendor}' succeeded - {result_summary}") | ||
|
|
||
| # Stopping logic: Stop after first successful vendor for single-vendor configs | ||
| # Multiple vendor configs (comma-separated) may want to collect from multiple sources | ||
| # Stopping logic: | ||
| # - Single vendor config: Stop after first success | ||
| # - Multi-vendor config: Stop after all primary vendors attempted | ||
| if len(primary_vendors) == 1: | ||
| print(f"DEBUG: Stopping after successful vendor '{vendor}' (single-vendor config)") | ||
| break | ||
| elif set(primary_vendors_attempted) == set(primary_vendors): | ||
| print(f"DEBUG: All primary vendors attempted ({', '.join(primary_vendors_attempted)}), stopping") | ||
| break | ||
| else: | ||
| print(f"FAILED: Vendor '{vendor}' produced no results") |
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.
There are a couple of issues with the current stopping logic for multi-vendor configurations:
- The check is inside the
if vendor_results:block, which means the loop only stops if the last primary vendor attempted was successful. The desired behavior is to stop after all primary vendors are attempted, regardless of success. - The
set(...) == set(...)comparison will not work correctly if the vendor configuration contains duplicate names (e.g.,'a,b,a'). The loop would stop prematurely because sets only contain unique elements.
The suggestion below fixes the first issue by moving the check. For the second issue, you might consider ensuring primary_vendors only contains unique entries when it's created, or using a different comparison method that handles duplicates (like collections.Counter).
| if vendor_results: | |
| results.extend(vendor_results) | |
| successful_vendor = vendor | |
| result_summary = f"Got {len(vendor_results)} result(s)" | |
| print(f"SUCCESS: Vendor '{vendor}' succeeded - {result_summary}") | |
| # Stopping logic: Stop after first successful vendor for single-vendor configs | |
| # Multiple vendor configs (comma-separated) may want to collect from multiple sources | |
| # Stopping logic: | |
| # - Single vendor config: Stop after first success | |
| # - Multi-vendor config: Stop after all primary vendors attempted | |
| if len(primary_vendors) == 1: | |
| print(f"DEBUG: Stopping after successful vendor '{vendor}' (single-vendor config)") | |
| break | |
| elif set(primary_vendors_attempted) == set(primary_vendors): | |
| print(f"DEBUG: All primary vendors attempted ({', '.join(primary_vendors_attempted)}), stopping") | |
| break | |
| else: | |
| print(f"FAILED: Vendor '{vendor}' produced no results") | |
| if vendor_results: | |
| results.extend(vendor_results) | |
| successful_vendor = vendor | |
| result_summary = f"Got {len(vendor_results)} result(s)" | |
| print(f"SUCCESS: Vendor '{vendor}' succeeded - {result_summary}") | |
| # Stopping logic: | |
| # - Single vendor config: Stop after first success | |
| if len(primary_vendors) == 1: | |
| print(f"DEBUG: Stopping after successful vendor '{vendor}' (single-vendor config)") | |
| break | |
| else: | |
| print(f"FAILED: Vendor '{vendor}' produced no results") | |
| # - Multi-vendor config: Stop after all primary vendors attempted | |
| if len(primary_vendors) > 1 and set(primary_vendors_attempted) == set(primary_vendors): | |
| print(f"DEBUG: All primary vendors attempted ({', '.join(primary_vendors_attempted)}), stopping") | |
| break |
When multiple primary vendors are configured (e.g., 'reddit,alpha_vantage'), the system now correctly stops after attempting all primary vendors instead of continuing through all fallback vendors. Changes: - Track which primary vendors have been attempted in a list - Add stopping condition when all primary vendors are attempted - Preserve existing single-vendor behavior (stop after first success) This prevents unnecessary API calls and ensures predictable behavior.
Tests verify: - Single vendor stops after first success - Multi-vendor stops after all primaries (even if they fail) - Fallback vendors are not attempted when primaries are configured - Tool-level config overrides category-level config Tests use pytest with fixtures and mocked vendors, can run without API keys in CI/CD. Run with: pytest tests/test_multi_vendor_routing.py -v
ca9b9f5 to
9080dbc
Compare
When multiple primary vendors are configured (e.g., 'reddit,alpha_vantage'), the system now correctly stops after attempting all primary vendors instead of continuing through all fallback vendors.
Changes:
This prevents unnecessary API calls and ensures predictable behavior.