Add inconsistency check pipeline #197
Conversation
WalkthroughThis pull request introduces a new pipeline for performing inconsistency checks in a programming exercise context. The changes span multiple files to implement a comprehensive feature that allows identifying and reporting inconsistencies between a problem statement and a template repository. The implementation includes creating new data transfer objects (DTOs), adding a pipeline execution route, defining a prompt for inconsistency analysis, and establishing a callback mechanism for tracking the pipeline's progress. Changes
Possibly related PRs
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: 6
🧹 Nitpick comments (11)
app/pipeline/inconsistenct_check_pipeline.py (5)
1-3: Code formatting required
The code must be reformatted to comply with Black style as indicated in the pipeline failures.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
4-8: Ensure dependencies are pinned
You might consider pinninglangchain_coreandlangsmithversions in your requirements to avoid unexpected breaking changes.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
25-43: Constructor clarity
The constructor is logically structured. However, adding a descriptive docstring at the class level would make usage expectations clearer.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
53-55: Validate exercise presence
It's good that you checkdto.exercise. Consider adding more context to the error message for easier debugging.- logger.error("Inconsistency check pipeline requires an exercise") + logger.error( + "Inconsistency check pipeline requires an exercise, but dto.exercise is missing. DTO contents: %s", + dto + )🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
77-77: Ensure final callback is always invoked
If an exception occurs before line 77, the callback might never fire. Consider using atry/finallyapproach to guarantee the pipeline updates its status.+ try: + ... + finally: + self.callback.done(final_result=response, tokens=self.tokens)🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
app/domain/status/inconsistency_check_status_update_dto.py (1)
1-5: Add a class-level docstring
A docstring explaining howresultis used in the context of status updates would make this DTO clearer.+ """ + This DTO updates the status and captures the final result of the inconsistency check pipeline. + """app/pipeline/prompts/inconsistency_check_prompts.py (1)
1-28: Correct grammatical repetition.Line 4 contains a repeated word “the the.” Removing the extra “the” improves readability. Also remember to run Black to address style warnings.
- The student will use the the template repository... + The student will use the template repository...app/web/status/status_update.py (1)
143-144: Confirm conditionally resetting suggestions.The conditionally set
suggestions = Nonelogic is acceptable, but verify thatsuggestionsis indeed optional or absent in certain DTOs. Otherwise, you might streamline with a typed field instead of relying onhasattr.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
app/web/routers/pipelines.py (1)
238-260: Thorough exception handling.This worker function properly logs and handles exceptions, transitioning the pipeline into an error state when needed. Consider adding unit tests to simulate failure scenarios.
🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
app/domain/inconsistency_check_pipeline_execution_dto.py (2)
6-6: Remove extra empty line.There's an unnecessary extra empty line that can be removed to maintain consistent spacing between imports and class definition.
from .data.programming_exercise_dto import ProgrammingExerciseDTO - class InconsistencyCheckPipelineExecutionDTO(BaseModel):
7-9: Add documentation and field descriptions.The class would benefit from:
- A docstring explaining its purpose and usage
- Field descriptions for better API documentation
- Optional validation rules if needed
class InconsistencyCheckPipelineExecutionDTO(BaseModel): - execution: PipelineExecutionDTO - exercise: ProgrammingExerciseDTO + """Data Transfer Object for inconsistency check pipeline execution. + + This DTO encapsulates the necessary data for executing an inconsistency check + between a programming exercise and its implementation. + """ + execution: PipelineExecutionDTO = Field( + description="Pipeline execution details and context" + ) + exercise: ProgrammingExerciseDTO = Field( + description="Programming exercise to check for inconsistencies" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/common/PipelineEnum.py(1 hunks)app/domain/__init__.py(1 hunks)app/domain/inconsistency_check_pipeline_execution_dto.py(1 hunks)app/domain/status/inconsistency_check_status_update_dto.py(1 hunks)app/pipeline/inconsistenct_check_pipeline.py(1 hunks)app/pipeline/prompts/inconsistency_check_prompts.py(1 hunks)app/web/routers/pipelines.py(4 hunks)app/web/status/status_update.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
app/domain/__init__.py
9-9: .inconsistency_check_pipeline_execution_dto.InconsistencyCheckPipelineExecutionDTO imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🪛 GitHub Actions: Run linters
app/domain/__init__.py
[warning] Code formatting does not comply with Black style. File needs reformatting.
app/web/status/status_update.py
[warning] Code formatting does not comply with Black style. File needs reformatting.
app/web/routers/pipelines.py
[warning] Code formatting does not comply with Black style. File needs reformatting.
app/pipeline/inconsistenct_check_pipeline.py
[warning] Code formatting does not comply with Black style. File needs reformatting.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Push Docker Image
- GitHub Check: Mend Security Check
🔇 Additional comments (13)
app/pipeline/inconsistenct_check_pipeline.py (4)
9-16: Imports appear consistent
All referenced imports appear to be used correctly in the code. No immediate concerns here.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
17-19: Logger usage is optimal
Usinglogger = logging.getLogger(__name__)follows standard best practices for Python logging.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
45-51: Docstring is thorough
The documentation for the__call__method is clear and comprehensive, explaining the pipeline steps precisely. Good job!🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
73-75: Thread-safety verification
If_append_tokenscan be called from multiple threads, appending toself.tokensmight lead to concurrency issues. Confirm usage patterns to avoid data races.✅ Verification successful
Thread safety confirmed: No concurrency issues with
_append_tokens
Each pipeline instance is created per request and runs in its own thread with isolated state. The_append_tokensmethod operates only on instance-specificself.tokenslist, making it thread-safe by design.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for concurrency references that might call _append_tokens in parallel. rg '_append_tokens' -A 10 ast-grep --pattern $'Thread($_, $_)'Length of output: 12367
Script:
#!/bin/bash # Check worker implementations and pipeline instance creation rg "run_.*_pipeline_worker" -A 10Length of output: 9183
Script:
#!/bin/bash # Check pipeline instantiation and storage patterns rg "= .*Pipeline\(" -A 5 rg "global.*pipeline" -iLength of output: 6948
🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
app/common/PipelineEnum.py (1)
17-17: New pipeline enumeration
AddingIRIS_INCONSISTENCY_CHECKaligns well with the existing naming convention.app/web/status/status_update.py (3)
15-15: Usage confirmed.This import is used later in the
InconsistencyCheckCallbackclass. No issues found.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
175-176: Same suggestion as above.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
281-301: New callback class looks good.The
InconsistencyCheckCallbackclass is clear, properly initializes stages, and logs errors. Consider adding docstrings or test coverage for completeness.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
app/web/routers/pipelines.py (4)
13-13: Import usage is valid.This import is used in the new worker function and route to handle the DTO.
🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
24-24: Import usage is valid.
InconsistencyCheckCallbackis instantiated in the worker function. No issues found.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
31-31: Import usage is valid.
InconsistencyCheckPipelineis properly invoked in the new worker function.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
338-345: Additional variant registration.The new pipeline variant is clearly defined here. Remember to run Black on this file to address style warnings.
🧰 Tools
🪛 GitHub Actions: Run linters
[warning] Code formatting does not comply with Black style. File needs reformatting.
app/domain/inconsistency_check_pipeline_execution_dto.py (1)
7-9: Well-structured DTO with clear purpose.The class is well-designed:
- Clear and specific naming that follows the established pattern
- Appropriate use of Pydantic for data validation
- Good separation of concerns by having a dedicated DTO for this pipeline type
FelixTJDietrich
force-pushed
the
feature/artemis-intelligence/inconsistency-check
branch
from
8f3f96e to
0c9cca4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/pipeline/inconsistency_check_pipeline.py (1)
1-74: Reformat using Black
The pipeline failures indicate that this file does not comply with Black. Please run Black to ensure consistent formatting.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] File needs to be reformatted using Black
app/web/routers/pipelines.py (2)
238-260: Check for variant usage
Withinrun_inconsistency_check_pipeline_worker, the_variantparameter is unused when constructing and running the pipeline. If variants matter for the inconsistency check, consider passing_varianttoInconsistencyCheckPipelinefor clarity or removing the parameter if it's unnecessary.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] File needs to be reformatted using Black
Line range hint
1-345: Reformat using Black
Lint checks indicate this file needs reformatting to comply with Black style guidelines.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] File needs to be reformatted using Black
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/pipeline/inconsistency_check_pipeline.py(1 hunks)app/pipeline/prompts/inconsistency_check_prompts.py(1 hunks)app/web/routers/pipelines.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/pipeline/prompts/inconsistency_check_prompts.py
🧰 Additional context used
🪛 GitHub Actions: Run linters
app/web/routers/pipelines.py
[warning] File needs to be reformatted using Black
app/pipeline/inconsistency_check_pipeline.py
[warning] File needs to be reformatted using Black
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Push Docker Image
🔇 Additional comments (3)
app/web/routers/pipelines.py (3)
Line range hint
13-31: Imports look good
All added imports forInconsistencyCheckPipelineExecutionDTO,InconsistencyCheckCallback, andInconsistencyCheckPipelineare consistent with usage in this file.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] File needs to be reformatted using Black
262-274: Route aligned with other endpoints
The new endpoint follows the established pattern of creating a thread to run the pipeline worker. This is consistent with the existing pipeline approach.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] File needs to be reformatted using Black
338-345: Variant listing is consistent
Listing a default variant for "INCONSISTENCY_CHECK" aligns with the approach for other pipeline features.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] File needs to be reformatted using Black
|
@FelixTJDietrich Can you review the conflicts, please? |
FelixTJDietrich
dismissed stale reviews from bassner and coderabbitai[bot]
via
5b370f6
|
@bassner should be good now |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/web/routers/pipelines.py (1)
325-334: Add logging for pipeline start similar to rewriting pipeline.For consistency with other routes (e.g., rewriting pipeline), add logging when the pipeline starts.
Apply this diff:
def run_inconsistency_check_pipeline( variant: str, dto: InconsistencyCheckPipelineExecutionDTO ): + logger.info(f"Inconsistency check pipeline started with variant: {variant} and dto: {dto}") thread = Thread(target=run_inconsistency_check_pipeline_worker, args=(dto, variant)) thread.start()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/common/PipelineEnum.py(1 hunks)app/domain/__init__.py(1 hunks)app/pipeline/inconsistency_check_pipeline.py(1 hunks)app/pipeline/prompts/inconsistency_check_prompts.py(1 hunks)app/web/routers/pipelines.py(6 hunks)app/web/status/status_update.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/common/PipelineEnum.py
- app/pipeline/prompts/inconsistency_check_prompts.py
- app/web/status/status_update.py
🧰 Additional context used
🪛 Ruff (0.8.2)
app/domain/__init__.py
10-10: .inconsistency_check_pipeline_execution_dto.InconsistencyCheckPipelineExecutionDTO imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Push Docker Image
- GitHub Check: Mend Security Check
🔇 Additional comments (4)
app/domain/__init__.py (1)
9-11: Verify the import's usage in the codebase.Let's confirm that this DTO is being used elsewhere in the codebase.
✅ Verification successful
Import is valid and actively used in the codebase
The
InconsistencyCheckPipelineExecutionDTOis properly used in multiple components:
- Used for type hints in
app/pipeline/inconsistency_check_pipeline.py- Used in multiple function signatures in
app/web/routers/pipelines.py🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of InconsistencyCheckPipelineExecutionDTO rg "InconsistencyCheckPipelineExecutionDTO" --type pythonLength of output: 88
Script:
#!/bin/bash # Search for both the class name and the file echo "=== Searching for class usage ===" rg "InconsistencyCheckPipelineExecutionDTO" echo -e "\n=== Searching for the file ===" fd "inconsistency_check_pipeline_execution_dto.py" echo -e "\n=== Searching for imports using ast-grep ===" ast-grep --pattern 'from $_ import InconsistencyCheckPipelineExecutionDTO'Length of output: 1265
🧰 Tools
🪛 Ruff (0.8.2)
10-10:
.inconsistency_check_pipeline_execution_dto.InconsistencyCheckPipelineExecutionDTOimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
app/pipeline/inconsistency_check_pipeline.py (2)
20-41:⚠️ Potential issueConsider adding error handling for callback initialization.
The callback is assigned directly without checking if it's None, which could lead to NullPointerException when calling callback methods.
Apply this diff to add error handling:
def __init__(self, callback: Optional[InconsistencyCheckCallback] = None): super().__init__(implementation_id="inconsistency_check_pipeline") completion_args = CompletionArguments(temperature=0, max_tokens=2000) self.llm = IrisLangchainChatModel( request_handler=CapabilityRequestHandler( requirements=RequirementList( gpt_version_equivalent=4.5, context_length=16385, ) ), completion_args=completion_args, ) self.prompt = PromptTemplate.from_template(basic_prompt) self.pipeline = self.prompt | self.llm | StrOutputParser() - self.callback = callback + self.callback = callback or NoOpInconsistencyCheckCallback() self.tokens = []Likely invalid or redundant comment.
42-71: 🛠️ Refactor suggestionAdd error handling for template repository processing.
The template repository processing could fail if the dictionary is empty or contains invalid entries.
Also, consider adding input validation for the problem statement.
Apply this diff to add validation:
if not dto.exercise: logger.error("Inconsistency check pipeline requires an exercise") raise ValueError("Exercise is required") + if not dto.exercise.problem_statement: + logger.error("Problem statement is required") + raise ValueError("Problem statement is required") + + if not dto.exercise.template_repository: + logger.error("Template repository is required") + raise ValueError("Template repository is required") + logger.info("Running inconsistency check pipeline...")app/web/routers/pipelines.py (1)
399-406: LGTM!The pipeline variant definition is consistent with other pipeline variants.
Motivation
We want to offer instructor assistance and make it easier for them to find inconsistencies within a programming exercise. The aim is to offer a more correct exercise to the students so that they are less confused and frustrated when solving the exercise.
Description
This pull request introduces a new pipeline for checking inconsistencies in programming exercises.
Right now it is just a simple prompt taking problem statement and the template files.
TODOs (potentially for follow-up PRs):
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Technical Improvements