Conversation
There was a problem hiding this comment.
Pull Request Overview
This is a substantial refactoring of the LLM conversation generation system that introduces a cleaner, more modular API architecture. The refactoring transitions from a single-purpose main file to a comprehensive conversation generation framework with improved configurability and better separation of concerns.
Key changes include:
- Replacement of monolithic
main_generate.pywith modulargenerate.pyfeaturing cursor-based configuration - Enhanced LLM interface with optional message parameters for more flexible conversation initiation
- Improved model configuration filtering and parameter handling in the factory pattern
- Updated CSV data structure with additional persona fields for richer conversation scenarios
Reviewed Changes
Copilot reviewed 41 out of 44 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| main_generate.py | Completely removed - replaced by new generate.py approach |
| generate.py | New main generation script with cursor-friendly configuration API |
| llm_clients/*.py | Updated interface with optional message parameters and streamlined model configuration |
| generate_conversations/runner.py | Refactored to use new configuration structure with enhanced logging and file organization |
| data/personas.csv | Enhanced with additional "Trajectory of sharing" field for richer persona behavior |
| model_config.json | Added support for new Claude Sonnet 4 model |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| config2 = load_prompt_config(self.agent_model_config["prompt_name"]) | ||
| agent = LLMFactory.create_llm( | ||
| model_name=self.agent_model_config["model"], | ||
| name=self.agent_model_config.pop("name"), |
There was a problem hiding this comment.
Using pop() on the agent_model_config dictionary will permanently remove the 'name' key, which could cause issues if the runner is used multiple times or if the configuration is reused elsewhere. Use dict.get() instead to safely retrieve the value without modifying the original dictionary.
| name=self.agent_model_config.pop("name"), | |
| name=self.agent_model_config.get("name"), |
llm_clients/claude_llm.py
Outdated
|
|
||
| # Handle None message by providing a default starter message | ||
| if message is None: | ||
| message = "Hello! How are you today?" |
There was a problem hiding this comment.
[nitpick] The hardcoded default message 'Hello! How are you today?' may not be appropriate for all conversation contexts, especially in mental health scenarios where a more neutral or contextually appropriate greeting would be better. Consider making this configurable or using a more generic starter.
| message = "Hello! How are you today?" | |
| message = self.default_starter_message |
| current_speaker = self.llm1 | ||
| next_speaker = self.llm2 | ||
| if initial_message is None: | ||
| current_message = 'Start the conversation based on the system prompt' |
There was a problem hiding this comment.
[nitpick] The instruction 'Start the conversation based on the system prompt' is exposed to the LLM and may appear unnatural in conversations. Consider using None directly or a more natural conversation starter that doesn't reference system implementation details.
| current_message = 'Start the conversation based on the system prompt' | |
| current_message = "" |
| """ | ||
| # Normalize model name to determine provider | ||
| model_lower = model_name.lower() | ||
| print(f"creating llm with {model_name}", system_prompt) |
There was a problem hiding this comment.
Debug print statement should be removed from production code or replaced with proper logging using the logging module to maintain clean output and proper log management.
| print(f"creating llm with {model_name}", system_prompt) | |
| logging.info(f"Creating LLM with model_name: {model_name}, system_prompt: {system_prompt}") |
mwilliford
left a comment
mwilliford
left a comment
There was a problem hiding this comment.
LGTM - given it is still forming itself.
Using Cursor to have a friendlier API