Improved consistency check #203
Conversation
WalkthroughThe update to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Pipeline
participant Solver
participant Prettify
Client->>Pipeline: Invoke __call__ with file paths
Pipeline->>Solver: Execute solver process to check consistency
Solver-->>Pipeline: Return raw consistency issues
Pipeline->>Prettify: Forward issues for formatting
Prettify-->>Pipeline: Return formatted summary response
Pipeline->>Client: Return final response after logging adjustments
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/pipeline/inconsistency_check_pipeline.py (1)
106-112: Consider allowing partial backtick removal.Currently, you remove the triple backticks only if the entire string begins and ends with them. If partial backticks occur or if the content is enclosed with some different formatting, they won’t be stripped. This may be intentional, but if you want to remove all extraneous code fences, consider a more flexible approach.
app/pipeline/prompts/inconsistency_check_prompts.py (1)
31-31: Remove trailing whitespace.Excess trailing whitespace triggers style warnings. Suggest removing it to adhere to linting rules.
Apply this minimal fix:
🧰 Tools
🪛 GitHub Actions: Run linters
[warning] 31-31: trailing whitespace
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/pipeline/inconsistency_check_pipeline.py(4 hunks)app/pipeline/prompts/inconsistency_check_prompts.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run linters
app/pipeline/prompts/inconsistency_check_prompts.py
[warning] 31-31: trailing whitespace
app/pipeline/inconsistency_check_pipeline.py
[warning] 4-4: 'langchain_core.runnables.RunnableParallel' imported but unused
🪛 Ruff (0.8.2)
app/pipeline/inconsistency_check_pipeline.py
4-4: langchain_core.runnables.RunnableParallel imported but unused
Remove unused import: langchain_core.runnables.RunnableParallel
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Push Docker Image
🔇 Additional comments (6)
app/pipeline/inconsistency_check_pipeline.py (4)
14-16: Importing new prompts looks good.This appropriately replaces the old prompt dependencies with the new prompts for solver and prettify.
26-28: New attributes for solver and prettify are well-defined.No concerns here; it’s straightforward and clear.
66-88: Logic for processing file paths and invoking the solver pipeline is clear.Combining the file sets from
template_repositoryandsolution_repositoryis a good approach to ensure all files are included in the checks.
90-100: Formatting of consistency issues before the prettify pipeline is coherent.The approach to create a structured string for each file is a nice touch for clarity downstream.
app/pipeline/prompts/inconsistency_check_prompts.py (2)
1-34: Implementation ofsolver_promptis well-structured.The detailed instructions and structured placeholders help ensure that consistency checks are processed clearly.
🧰 Tools
🪛 GitHub Actions: Run linters
[warning] 31-31: trailing whitespace
36-62:prettify_promptupdates look solid.Defining a clear structure for summarizing consistency issues fosters well-organized results.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/pipeline/prompts/inconsistency_check_prompts.py (1)
36-62: Consider adding validation for consistency issues format.The
prettify_promptexpects consistency issues in a specific format (as seen in lines 91-96 of the pipeline), but this format requirement isn't explicitly mentioned in the prompt's response section.Add format requirements to the instruction section:
<Response> Respond with a summary of the consistency issues found in the exercise. \ -Just provide the easily digestible formatted markdown without other explanations. +Just provide the easily digestible formatted markdown without other explanations. \ +The consistency issues are provided in <PotentialFileIssues> tags with filePath attributes. </Response>app/pipeline/inconsistency_check_pipeline.py (1)
106-112: Consider using regex for markdown cleanup.The current string manipulation for removing markdown syntax is brittle and might miss edge cases.
Consider using regex for more robust cleanup:
-if result.startswith("```") and result.endswith("```"): - result = result[3:-3] - if result.startswith("markdown"): - result = result[8:] - result = result.strip() +import re +result = re.sub(r'^```(?:markdown)?\n?(.*?)\n?```$', r'\1', result.strip(), flags=re.DOTALL)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/pipeline/inconsistency_check_pipeline.py(4 hunks)app/pipeline/prompts/inconsistency_check_prompts.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Push Docker Image
🔇 Additional comments (3)
app/pipeline/prompts/inconsistency_check_prompts.py (1)
1-34: LGTM! Well-structured solver prompt with clear instructions.The
solver_promptis well-organized with clear sections and explicit instructions. The context about being an expert instructor and the explanation of exercise components helps set clear expectations.app/pipeline/inconsistency_check_pipeline.py (2)
91-102: LGTM! Clear formatting of consistency issues.The formatting of consistency issues using XML-like tags with file paths is clean and structured.
66-88: 🛠️ Refactor suggestionConsider adding error handling for file mismatches.
The code handles missing files by using "empty file" as a default, but this might mask potential issues where files are unexpectedly missing from either repository.
Add validation and logging for file mismatches:
file_paths = set(dto.exercise.template_repository.keys()) | set( dto.exercise.solution_repository.keys() ) +template_only = set(dto.exercise.template_repository.keys()) - set(dto.exercise.solution_repository.keys()) +solution_only = set(dto.exercise.solution_repository.keys()) - set(dto.exercise.template_repository.keys()) +if template_only or solution_only: + logger.warning( + f"File mismatch detected - Template only: {template_only}, Solution only: {solution_only}" + )
FelixTJDietrich
temporarily deployed
to
Iris Test
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/pipeline/inconsistency_check_pipeline.py (1)
113-114:⚠️ Potential issueAdd null check for callback.
The callback is used without checking if it's None, which could lead to NullPointerException.
- self._append_tokens(self.llm.tokens, PipelineEnum.IRIS_INCONSISTENCY_CHECK) - self.callback.done(final_result=result, tokens=self.tokens) + self._append_tokens(self.llm.tokens, PipelineEnum.IRIS_INCONSISTENCY_CHECK) + if self.callback: + self.callback.done(final_result=result, tokens=self.tokens) + else: + logger.warning("No callback provided, skipping status update")
🧹 Nitpick comments (1)
app/pipeline/inconsistency_check_pipeline.py (1)
42-47: Add error handling for prompt template initialization.The prompt template initialization could fail if the templates are malformed. Consider adding try-catch blocks to handle potential errors gracefully.
- self.solver_prompt = PromptTemplate.from_template(solver_prompt) - self.solver = self.solver_prompt | self.llm - - self.prettify_prompt = PromptTemplate.from_template(prettify_prompt) - self.prettify = self.prettify_prompt | self.llm + try: + self.solver_prompt = PromptTemplate.from_template(solver_prompt) + self.solver = self.solver_prompt | self.llm + + self.prettify_prompt = PromptTemplate.from_template(prettify_prompt) + self.prettify = self.prettify_prompt | self.llm + except Exception as e: + logger.error(f"Failed to initialize prompt templates: {e}") + raise RuntimeError("Failed to initialize inconsistency check pipeline") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/pipeline/inconsistency_check_pipeline.py(4 hunks)
🔇 Additional comments (1)
app/pipeline/inconsistency_check_pipeline.py (1)
2-2: LGTM! Clean import structure and well-defined class attributes.The imports and class attributes are properly organized and align well with the new two-step pipeline implementation.
Also applies to: 4-6, 14-17, 26-27
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/pipeline/inconsistency_check_pipeline.py (2)
68-90:⚠️ Potential issueAdd error handling for file processing.
The file processing lacks error handling for potential failures.
This issue was previously identified. Please refer to the existing comment for the suggested fix.
🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code does not adhere to formatting standards. Please format the code using Black.
119-120:⚠️ Potential issueAdd error handling for callback.
The callback is used without checking if it's None.
This issue was previously identified. Please refer to the existing comment for the suggested fix.
🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code does not adhere to formatting standards. Please format the code using Black.
🧹 Nitpick comments (3)
app/pipeline/prompts/inconsistency_check_prompts.py (1)
36-62: Consider adding validation criteria for consistency issues.While the prettify prompt is well-structured, it could benefit from explicit criteria for what constitutes a significant consistency issue worth reporting.
Add validation criteria section to the instruction:
Parts of a programming exercise: - Problem statement: The description of the exercise containing tasks that the student needs to solve. - Template repository: The starting point from which the student will start solving the exercise. - Solution repository: The sample solution set by the instructor to compare the student's solution against. + +Consider the following criteria when summarizing consistency issues: + - Impact on student understanding + - Potential to cause confusion + - Technical correctness + - Alignment with exercise objectivesapp/pipeline/inconsistency_check_pipeline.py (2)
28-30: Add type hints for Runnable attributes.The solver and prettify attributes should have their types explicitly defined.
- solver: Runnable - prettify: Runnable + solver: Runnable[dict, str] + prettify: Runnable[dict, str]🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code does not adhere to formatting standards. Please format the code using Black.
109-118: Consider moving result formatting to a separate method.The result formatting logic with regex operations should be extracted into a dedicated method for better maintainability.
+ def _format_result(self, result: str) -> str: + """Format the LLM response by removing markdown artifacts.""" + result = result.strip() + if result.startswith("```") and result.endswith("```"): + result = result[3:-3] + if result.startswith("markdown"): + result = result[8:] + result = result.strip() + + # Remove headings + result = re.sub(r"^#\s.*?\n", "", result) + result = re.sub(r"^#+.*?Summary of Consistency Issues\s*\n", "", result) + return result def __call__(self, dto: InconsistencyCheckPipelineExecutionDTO, **kwargs): # ... existing code ... - if result.startswith("```") and result.endswith("```"): - result = result[3:-3] - if result.startswith("markdown"): - result = result[8:] - result = result.strip() - - # Remove first heading or heading containing 'Summary of Consistency Issues' - result = re.sub(r"^#\s.*?\n", "", result) - result = re.sub(r"^#+.*?Summary of Consistency Issues\s*\n", "", result) + result = self._format_result(result)🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code does not adhere to formatting standards. Please format the code using Black.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/pipeline/inconsistency_check_pipeline.py(4 hunks)app/pipeline/prompts/inconsistency_check_prompts.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run linters
app/pipeline/inconsistency_check_pipeline.py
[warning] Code does not adhere to formatting standards. Please format the code using Black.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Push Docker Image
🔇 Additional comments (2)
app/pipeline/prompts/inconsistency_check_prompts.py (1)
1-34: LGTM! Well-structured solver prompt with clear instructions.The solver prompt effectively:
- Establishes the expert instructor role
- Clearly defines exercise components
- Specifies the file-by-file approach
- Provides clear response format expectations
app/pipeline/inconsistency_check_pipeline.py (1)
1-19: Format imports according to PEP 8.The imports need to be formatted according to PEP 8 standards, as indicated by the pipeline failure.
Run the following script to check import formatting:
🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code does not adhere to formatting standards. Please format the code using Black.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/pipeline/inconsistency_check_pipeline.py (2)
68-90:⚠️ Potential issueAdd error handling for file processing.
This is a duplicate of a previous review comment. The file processing still lacks proper error handling.
119-120:⚠️ Potential issueAdd null check for callback.
This is a duplicate of a previous review comment. The callback is still used without checking if it's None.
🧹 Nitpick comments (2)
app/pipeline/prompts/inconsistency_check_prompts.py (1)
36-62: Consider adding validation criteria for consistency issues.While the prettify prompt is well-structured, it could benefit from explicit criteria for what constitutes a significant consistency issue worth reporting. This would help standardize the output across different files.
Add validation criteria to the instruction section:
without any errors or inconsistencies that might confuse students. In a previous step you already found potential consistency issues as part of the exercise creation process on a file \ level. Now, you need to summarize the issues found in the exercise so the teaching assistants can fix them. + +Consider the following criteria when summarizing issues: + - Impact on student understanding + - Technical correctness + - Alignment with exercise objectives + - Clarity of instructionsapp/pipeline/inconsistency_check_pipeline.py (1)
109-117: Consider moving markdown cleanup to a separate method.The markdown cleanup logic could be reused elsewhere and would benefit from being in a dedicated method with proper documentation.
+ def _clean_markdown_response(self, result: str) -> str: + """ + Cleans up markdown formatting from LLM response. + + Args: + result: Raw markdown response from LLM + + Returns: + Cleaned markdown string + """ + result = result.strip() + if result.startswith("```") and result.endswith("```"): + result = result[3:-3] + if result.startswith("markdown"): + result = result[8:] + result = result.strip() + + # Remove headings + result = re.sub(r"^#\s.*?\n", "", result) + result = re.sub(r"^#+.*?Summary of Consistency Issues\s*\n", "", result) + return result
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/pipeline/inconsistency_check_pipeline.py(4 hunks)app/pipeline/prompts/inconsistency_check_prompts.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Push Docker Image
🔇 Additional comments (1)
app/pipeline/prompts/inconsistency_check_prompts.py (1)
1-34: LGTM! Well-structured solver prompt with clear instructions.The solver prompt effectively implements the PR's objective of granular file-level consistency checking. The instructions are clear and the structure with XML-like tags makes it easy to parse.
FelixTJDietrich
temporarily deployed
to
Iris Test
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/pipeline/inconsistency_check_pipeline.py (1)
119-120:⚠️ Potential issueAdd null check for callback.
The callback is still being used without checking if it's None.
This issue was previously identified. Please refer to the earlier review comment for the suggested fix.
🧹 Nitpick comments (4)
app/pipeline/prompts/inconsistency_check_prompts.py (3)
1-34: Enhance prompt guidance for consistency checks.The solver prompt could be more effective by including:
- Examples of common consistency issues to look for (e.g., mismatched method signatures, inconsistent variable names)
- Specific criteria for what constitutes a consistency issue
Parts of a programming exercise: - Problem statement: The description of the exercise containing tasks that the student needs to solve. - Template repository: The starting point from which the student will start solving the exercise. - Solution repository: The sample solution set by the instructor to compare the student's solution against. +Common consistency issues to look for: +- Method signatures in template don't match the problem statement +- Variable names differ between problem statement and template +- Missing or extra methods compared to what's described +- Inconsistent parameter types or return types +- Template contains solution code or hints not mentioned in problem
29-33: Specify expected markdown structure for responses.The response section should provide a clear template for how consistency issues should be formatted.
<Response> Respond with any potential consistency issues found in the exercise formatted in markdown. \ Just provide the easily digestible formatted markdown without other explanations. It is fine to provide no issues if \ -you are confident that the files are consistent. +you are confident that the files are consistent. + +Expected markdown format: +## Consistency Issues + +### Issue 1: [Brief Issue Title] +- **Location**: [Specific line or section reference] +- **Description**: [Clear explanation of the inconsistency] +- **Impact**: [How this might affect students] + +If no issues found: +No consistency issues detected. </Response>
36-63: Enhance prettify prompt with prioritization guidance.The prettify prompt could be more effective by including instructions for:
- Prioritizing issues (e.g., critical vs minor)
- Grouping related issues
- Handling multiple issues from the same file
Respond with a summary of the consistency issues found in the exercise, stay specific and clear so the issues can be \ -easily fixed by the teaching assistants. Make it clear which file path contains the issues. Just provide the easily \ -digestible formatted markdown without other explanations. +easily fixed by the teaching assistants. Follow these guidelines: +- Prioritize issues as Critical, Important, or Minor +- Group related issues together +- For files with multiple issues, list them in order of priority +- Make it clear which file path contains the issues +Just provide the easily digestible formatted markdown without other explanations.app/pipeline/inconsistency_check_pipeline.py (1)
28-29: Add specific type hints for Runnable attributes.The
solverandprettifyattributes would benefit from more specific type hints.- solver: Runnable - prettify: Runnable + solver: Runnable[dict[str, str], str] + prettify: Runnable[dict[str, str], str]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/pipeline/inconsistency_check_pipeline.py(4 hunks)app/pipeline/prompts/inconsistency_check_prompts.py(1 hunks)
🔇 Additional comments (1)
app/pipeline/inconsistency_check_pipeline.py (1)
68-90: 🛠️ Refactor suggestionAdd validation and error handling for file processing.
The file processing logic should validate file contents and handle potential errors.
file_paths = set(dto.exercise.template_repository.keys()) | set( dto.exercise.solution_repository.keys() ) + if not file_paths: + logger.warning("No files found to process") + return "No files available for consistency check." + solver_inputs = [ { "file_path": file_path, "problem_statement": dto.exercise.problem_statement, - "template_file": dto.exercise.template_repository.get( - file_path, "no file found" - ), - "solution_file": dto.exercise.solution_repository.get( - file_path, "no file found" - ), + "template_file": ( + content if (content := dto.exercise.template_repository.get(file_path)) + else "File not found in template repository" + ), + "solution_file": ( + content if (content := dto.exercise.solution_repository.get(file_path)) + else "File not found in solution repository" + ), } for file_path in file_paths ] - file_responses = self.solver.map().invoke(solver_inputs) - consistency_issues = { - file_path: response.content - for file_path, response in zip(file_paths, file_responses) - } + try: + file_responses = self.solver.map().invoke(solver_inputs) + consistency_issues = { + file_path: response.content + for file_path, response in zip(file_paths, file_responses) + if response and hasattr(response, 'content') + } + except Exception as e: + logger.error(f"Error processing files: {e}") + raise RuntimeError("Failed to process files for consistency check") from eLikely invalid or redundant comment.
Previously, the inconsistency check only did one LLM call with all template files.
Now we are doing a LLM call per template + solution file to find consistency issues on a per file basis. Afterwards, we are prettifying the responses by using a final LLM call that summarizes the issues.
Summary by CodeRabbit
New Features
Refactor