-
Notifications
You must be signed in to change notification settings - Fork 1k
fix: add currying to question_answering_non_parallel #602
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: add currying to question_answering_non_parallel #602
Conversation
…l to avoid additional params
WalkthroughThis pull request updates the answer generation module by modifying function signatures and call flows. In the executor, the callable in the engine options is updated to expect two string arguments instead of one, and the method no longer accepts a separate system prompt parameter. Meanwhile, the run module now wraps the answer resolver in a lambda that injects the system prompt at call time. These changes adjust how queries and prompts are passed without altering core functionality. Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (30)
🔇 Additional comments (3)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cognee/eval_framework/answer_generation/answer_generation_executor.py (1)
26-26: Consider updating parameter type annotation for clarity.While functionally correct, there's a slight discrepancy between the type annotation and the actual usage pattern. The type annotation indicates a function that takes only one string parameter, but the actual function being passed from run_question_answering_module.py is a curried function that internally calls a two-parameter function.
Consider revising the type annotation to make this pattern more explicit, possibly with a comment:
- answer_resolver: Callable[[str], Awaitable[List[str]]], + answer_resolver: Callable[[str], Awaitable[List[str]]], # Curried function that handles system_prompt internally
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cognee/eval_framework/answer_generation/answer_generation_executor.py(2 hunks)cognee/eval_framework/answer_generation/run_question_answering_module.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (32)
- GitHub Check: run_multimedia_example_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_simple_example_test / test
- GitHub Check: run_networkx_metrics_test / test
- GitHub Check: Test on macos-15
- GitHub Check: run_eval_framework_test / test
- GitHub Check: Test on macos-13
- GitHub Check: run_notebook_test / test
- GitHub Check: run_dynamic_steps_example_test / test
- GitHub Check: Test on macos-15
- GitHub Check: run_notebook_test / test
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: Test on macos-13
- GitHub Check: Test on macos-15
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: test
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: test
- GitHub Check: Test on macos-13
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: windows-latest
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: test
- GitHub Check: run_simple_example_test
- GitHub Check: lint (ubuntu-latest, 3.11.x)
- GitHub Check: lint (ubuntu-latest, 3.10.x)
- GitHub Check: docker-compose-test
- GitHub Check: Build Cognee Backend Docker App Image
🔇 Additional comments (3)
cognee/eval_framework/answer_generation/run_question_answering_module.py (1)
51-53: Excellent implementation of lambda currying for the answer_resolver.The lambda function elegantly injects the system_prompt parameter, simplifying the interface between run_question_answering and the question_answering_non_parallel method. This approach correctly implements the PR objective of introducing lambda currying to streamline the function calls.
cognee/eval_framework/answer_generation/answer_generation_executor.py (2)
5-5: Type signature update correctly reflects the enhanced callable interface.The updated type signature now accurately indicates that the callables in the question_answering_engine_options dictionary accept two string parameters instead of one, which aligns perfectly with how the functions are actually defined in the code.
33-33: Method call simplified as a result of lambda currying.This simplification is a direct benefit of the lambda currying approach implemented in the run_question_answering module. The code is now cleaner as it no longer needs to handle the system_prompt parameter explicitly at this level.
…l to avoid additional params
Introduces lambda currying in question answering non parallel function to avoid unnecessary params
DCO Affirmation
I affirm that all code in every commit of this pull request conforms to the terms of the Topoteretes Developer Certificate of Origin
Summary by CodeRabbit