From a71756dc136507f59cecaa5dc9f32d34670a0c72 Mon Sep 17 00:00:00 2001 From: Leon Laurin Wehrhahn <58460654+LeonWehrhahn@users.noreply.github.com> Date: Sun, 8 Dec 2024 16:53:15 +0100 Subject: [PATCH] Modeling Exercises: Refactor Feedback Reference (#354) --- athena/athena/models/db_modeling_feedback.py | 5 +- athena/athena/schemas/modeling_feedback.py | 7 +-- llm_core/llm_core/models/openai.py | 2 +- .../apollon_json_transformer.py | 16 ++--- .../apollon_transformer/parser/element.py | 61 +++++++++++++++++-- .../apollon_transformer/parser/uml_parser.py | 28 ++++++++- .../module_modeling_llm/config.py | 2 +- .../models/assessment_model.py | 4 +- .../models/exercise_model.py | 5 +- .../prompts/graded_feedback_prompt.py | 12 +++- .../utils/convert_to_athana_feedback_model.py | 35 ++++++----- .../utils/get_exercise_model.py | 5 +- 12 files changed, 136 insertions(+), 46 deletions(-) diff --git a/athena/athena/models/db_modeling_feedback.py b/athena/athena/models/db_modeling_feedback.py index 142a59911..700ad068a 100644 --- a/athena/athena/models/db_modeling_feedback.py +++ b/athena/athena/models/db_modeling_feedback.py @@ -1,6 +1,6 @@ from typing import Optional -from sqlalchemy import Column, ForeignKey, JSON +from sqlalchemy import Column, ForeignKey, JSON, String from sqlalchemy.orm import relationship from athena.database import Base @@ -11,7 +11,8 @@ class DBModelingFeedback(DBFeedback, Base): __tablename__ = "modeling_feedbacks" - element_ids: Optional[list[str]] = Column(JSON) # type: ignore + element_ids: Optional[list[str]] = Column(JSON) # type: ignore # Todo: Remove after adding migrations to athena + reference: Optional[str] = Column(String, nullable=True) # type: ignore exercise_id = Column(BigIntegerWithAutoincrement, ForeignKey("modeling_exercises.id", ondelete="CASCADE"), index=True) submission_id = Column(BigIntegerWithAutoincrement, ForeignKey("modeling_submissions.id", ondelete="CASCADE"), index=True) diff --git a/athena/athena/schemas/modeling_feedback.py b/athena/athena/schemas/modeling_feedback.py index f77322f43..99fae7caf 100644 --- a/athena/athena/schemas/modeling_feedback.py +++ b/athena/athena/schemas/modeling_feedback.py @@ -1,11 +1,10 @@ -from typing import Optional, List - +from typing import List, Optional from pydantic import Field - from .feedback import Feedback class ModelingFeedback(Feedback): """Feedback on a modeling exercise.""" - element_ids: Optional[List[str]] = Field([], description="referenced diagram element IDs", example=["id_1"]) + element_ids: Optional[List[str]] = Field([], description="referenced diagram element IDs", example=["id_1"]) # Todo: Remove after adding migrations to athena + reference: Optional[str] = Field(None, description="reference to the diagram element", example="ClassAttribute:5a337bdf-da00-4bd0-a6f0-78ba5b84330e") \ No newline at end of file diff --git a/llm_core/llm_core/models/openai.py b/llm_core/llm_core/models/openai.py index d12df22d4..7bcc0f11f 100644 --- a/llm_core/llm_core/models/openai.py +++ b/llm_core/llm_core/models/openai.py @@ -67,7 +67,7 @@ class OpenAIModelConfig(ModelConfig): model_name: OpenAIModel = Field(default=default_openai_model, # type: ignore description="The name of the model to use.") - max_tokens: PositiveInt = Field(1000, description="""\ + max_tokens: PositiveInt = Field(4000, description="""\ The maximum number of [tokens](https://platform.openai.com/tokenizer) to generate in the chat completion. The total length of input tokens and generated tokens is limited by the model's context length. \ diff --git a/modules/modeling/module_modeling_llm/module_modeling_llm/apollon_transformer/apollon_json_transformer.py b/modules/modeling/module_modeling_llm/module_modeling_llm/apollon_transformer/apollon_json_transformer.py index 3d67196df..2142a2cdb 100644 --- a/modules/modeling/module_modeling_llm/module_modeling_llm/apollon_transformer/apollon_json_transformer.py +++ b/modules/modeling/module_modeling_llm/module_modeling_llm/apollon_transformer/apollon_json_transformer.py @@ -6,7 +6,7 @@ class ApollonJSONTransformer: @staticmethod - def transform_json(model: str) -> tuple[str, dict[str, str], str]: + def transform_json(model: str) -> tuple[str, dict[str, str], str, dict[str, str]]: """ Serialize a given Apollon diagram model to a string representation. This method converts the UML diagram model into a format similar to mermaid syntax, called "apollon". @@ -25,11 +25,11 @@ def transform_json(model: str) -> tuple[str, dict[str, str], str]: # Convert the UML diagram to the apollon representation apollon_representation = parser.to_apollon() - # Extract elements and relations with their corresponding IDs and names - names = { - **{element['name']: element['id'] for element in parser.get_elements()}, - **{relation['name']: relation['id'] for relation in parser.get_relations()} - } - - return apollon_representation, names, diagram_type + # Get the mapping of element, method, and attribute names to their corresponding IDs + # This is used to resolve references to as the apollon representation only contains names and not IDs + names = parser.get_element_id_mapping() + + id_type_mapping = parser.get_id_to_type_mapping() + + return apollon_representation, names, diagram_type, id_type_mapping \ No newline at end of file diff --git a/modules/modeling/module_modeling_llm/module_modeling_llm/apollon_transformer/parser/element.py b/modules/modeling/module_modeling_llm/module_modeling_llm/apollon_transformer/parser/element.py index 415b586a6..ef9c52a79 100644 --- a/modules/modeling/module_modeling_llm/module_modeling_llm/apollon_transformer/parser/element.py +++ b/modules/modeling/module_modeling_llm/module_modeling_llm/apollon_transformer/parser/element.py @@ -1,4 +1,7 @@ -from typing import Dict, Any, List, Optional +# apollon_transformer/parser/element.py + +from typing import Dict, Any, List, Optional, Tuple +from string import ascii_uppercase class Element: """ @@ -17,15 +20,61 @@ def __init__(self, data: Dict[str, Any], element_dict: Optional[Dict[str, Any]] self.method_refs: List[str] = data.get('methods', []) self.attributes: List[str] = [] self.methods: List[str] = [] + self.attribute_id_mapping: Dict[str, str] = {} + self.method_id_mapping: Dict[str, str] = {} if element_dict is not None: self.resolve_references(element_dict) def resolve_references(self, element_dict: Dict[str, Any]): """ - Resolve attribute and method references using the provided element dictionary. The json data contains only references to other elements that represent attributes and methods. This method resolves these references to the actual names of the attributes and methods by looking up the corresponding elements via their IDs in the provided element dictionary. + Resolve attribute and method references using the provided element dictionary. + Ensures uniqueness among attribute and method names within the class. """ - self.attributes = [element_dict[ref].get("name", "") for ref in self.attribute_refs if ref in element_dict] - self.methods = [element_dict[ref].get('name', '') for ref in self.method_refs if ref in element_dict] + # Resolve attributes + self.attributes, self.attribute_id_mapping = self._resolve_uniqueness( + self.attribute_refs, element_dict) + + # Resolve methods + self.methods, self.method_id_mapping = self._resolve_uniqueness( + self.method_refs, element_dict) + + def _resolve_uniqueness( + self, refs: List[str], element_dict: Dict[str, Any] + ) -> Tuple[List[str], Dict[str, str]]: + name_counts: Dict[str, int] = {} + unique_full_names: List[str] = [] + id_mapping: Dict[str, str] = {} + for ref in refs: + if ref in element_dict: + full_name = element_dict[ref].get("name", "") + simplified_name = self.extract_name(full_name) + count = name_counts.get(simplified_name, 0) + if count > 0: + suffix = f"#{ascii_uppercase[count - 1]}" + simplified_name_with_suffix = f"{simplified_name}{suffix}" + else: + simplified_name_with_suffix = simplified_name + name_counts[simplified_name] = count + 1 + unique_full_names.append(full_name) + id_mapping[simplified_name_with_suffix] = ref + return unique_full_names, id_mapping + + @staticmethod + def extract_name(full_name: str) -> str: + """ + Extracts the simplified name from the full attribute or method name. + Removes visibility symbols, type annotations, and parameters. + """ + # Remove visibility symbols and leading/trailing spaces + name = full_name.lstrip('+-#~ ').strip() + # For attributes, split on ':' + if ':' in name: + name = name.split(':')[0].strip() + # For methods, split on '(' + elif '(' in name: + name = name.split('(')[0].strip() + return name + def to_apollon(self) -> str: parts = [f"[{self.type}] {self.name}"] @@ -41,6 +90,6 @@ def to_apollon(self) -> str: parts.append("{\n" + "\n".join(details) + "\n}") return " ".join(parts) - + def __getitem__(self, key): - return self.__dict__[key] \ No newline at end of file + return self.__dict__[key] diff --git a/modules/modeling/module_modeling_llm/module_modeling_llm/apollon_transformer/parser/uml_parser.py b/modules/modeling/module_modeling_llm/module_modeling_llm/apollon_transformer/parser/uml_parser.py index 57b9a63ad..04b6ae6bc 100644 --- a/modules/modeling/module_modeling_llm/module_modeling_llm/apollon_transformer/parser/uml_parser.py +++ b/modules/modeling/module_modeling_llm/module_modeling_llm/apollon_transformer/parser/uml_parser.py @@ -93,4 +93,30 @@ def get_elements(self) -> List[Element]: return self.elements def get_relations(self) -> List[Relation]: - return self.relations \ No newline at end of file + return self.relations + + def get_element_id_mapping(self) -> Dict[str, str]: + """ + Creates a mapping from element names to their IDs, including attributes and methods. + """ + mapping = {} + for element in self.elements: + mapping[element.name] = element.id + for simplified_name_with_suffix, attr_id in element.attribute_id_mapping.items(): + mapping[f"{element.name}.{simplified_name_with_suffix}"] = attr_id + for simplified_name_with_suffix, method_id in element.method_id_mapping.items(): + mapping[f"{element.name}.{simplified_name_with_suffix}"] = method_id + for relation in self.relations: + mapping[relation.name] = relation.id + return mapping + + def get_id_to_type_mapping(self) -> Dict[str, str]: + """ + Creates a mapping from IDs to their types, including attributes and methods. + """ + mapping = {} + for element in self.data['elements'].values(): + mapping[element['id']] = element['type'] + for relation in self.data['relationships'].values(): + mapping[relation['id']] = relation['type'] + return mapping \ No newline at end of file diff --git a/modules/modeling/module_modeling_llm/module_modeling_llm/config.py b/modules/modeling/module_modeling_llm/module_modeling_llm/config.py index 169ee047a..c7f0f4ad0 100644 --- a/modules/modeling/module_modeling_llm/module_modeling_llm/config.py +++ b/modules/modeling/module_modeling_llm/module_modeling_llm/config.py @@ -43,7 +43,7 @@ class GenerateSuggestionsPrompt(BaseModel): class BasicApproachConfig(BaseModel): """This approach uses a LLM with a single prompt to generate feedback in a single step.""" - max_input_tokens: int = Field(default=3000, description="Maximum number of tokens in the input prompt.") + max_input_tokens: int = Field(default=5000, description="Maximum number of tokens in the input prompt.") model: ModelConfigType = Field(default=DefaultModelConfig()) # type: ignore generate_suggestions_prompt: GenerateSuggestionsPrompt = Field(default=GenerateSuggestionsPrompt()) diff --git a/modules/modeling/module_modeling_llm/module_modeling_llm/models/assessment_model.py b/modules/modeling/module_modeling_llm/module_modeling_llm/models/assessment_model.py index 9ccc1933d..29ccdb55a 100644 --- a/modules/modeling/module_modeling_llm/module_modeling_llm/models/assessment_model.py +++ b/modules/modeling/module_modeling_llm/module_modeling_llm/models/assessment_model.py @@ -1,10 +1,10 @@ -from typing import List, Optional, Sequence +from typing import Optional, Sequence from pydantic import BaseModel, Field class FeedbackModel(BaseModel): title: str = Field(description="Very short title, i.e. feedback category or similar", example="Logic Error") description: str = Field(description="Feedback description") - element_names: Optional[List[str]] = Field(description="Referenced diagram element names, and relations (R) or empty if unreferenced") + element_name: Optional[str] = Field(description="Referenced diagram element, attribute names, and relations (use format: , ., ., R), or leave empty if unreferenced") credits: float = Field(0.0, description="Number of points received/deducted") grading_instruction_id: int = Field( description="ID of the grading instruction that was used to generate this feedback" diff --git a/modules/modeling/module_modeling_llm/module_modeling_llm/models/exercise_model.py b/modules/modeling/module_modeling_llm/module_modeling_llm/models/exercise_model.py index 3a6f0672f..1d9eef7c3 100644 --- a/modules/modeling/module_modeling_llm/module_modeling_llm/models/exercise_model.py +++ b/modules/modeling/module_modeling_llm/module_modeling_llm/models/exercise_model.py @@ -1,4 +1,4 @@ -from typing import Optional +from typing import Optional, Dict from pydantic import BaseModel class ExerciseModel(BaseModel): @@ -11,4 +11,5 @@ class ExerciseModel(BaseModel): grading_instructions: Optional[str] = None submission_uml_type: str transformed_example_solution: Optional[str] = None - element_id_mapping: dict[str, str] \ No newline at end of file + element_id_mapping: Dict[str, str] + id_type_mapping: Dict[str, str] \ No newline at end of file diff --git a/modules/modeling/module_modeling_llm/module_modeling_llm/prompts/graded_feedback_prompt.py b/modules/modeling/module_modeling_llm/module_modeling_llm/prompts/graded_feedback_prompt.py index f2fa1390f..71662c84b 100644 --- a/modules/modeling/module_modeling_llm/module_modeling_llm/prompts/graded_feedback_prompt.py +++ b/modules/modeling/module_modeling_llm/module_modeling_llm/prompts/graded_feedback_prompt.py @@ -42,9 +42,15 @@ class GradedFeedbackInputs(BaseModel): {example_solution} Important: -Make sure to provide detailed feedback for each criterion. Always try to be as specific as possible. -Also make sure your feedback adds up to the correct number of points. If there are n points available and everything is correct, then the feedback should add up to n points. -Deeply think about the diagram and what the student potentially missed, misunderstood or mixed up. +- Make sure to provide detailed feedback for each criterion. Always try to be as specific as possible. +- Also make sure your feedback adds up to the correct number of points. If there are n points available and everything is correct, then the feedback should add up to n points. +- Deeply think about the diagram and what the student potentially missed, misunderstood, or mixed up. +- For the `element_name` field in the output, reference the specific diagram element, attribute, method, or relation related to the feedback. Use the following formats: + - For classes or elements: `` + - For attributes: `.` + - For methods: `.` + - For relations: `R` (e.g., `R1`, `R2`) +- If the feedback is not related to a specific element, leave the `element_name` field empty. The submission uses the following UML diagram format: diff --git a/modules/modeling/module_modeling_llm/module_modeling_llm/utils/convert_to_athana_feedback_model.py b/modules/modeling/module_modeling_llm/module_modeling_llm/utils/convert_to_athana_feedback_model.py index 06b4b5a66..4ade6b3c7 100644 --- a/modules/modeling/module_modeling_llm/module_modeling_llm/utils/convert_to_athana_feedback_model.py +++ b/modules/modeling/module_modeling_llm/module_modeling_llm/utils/convert_to_athana_feedback_model.py @@ -6,38 +6,45 @@ def convert_to_athana_feedback_model( - feedback_result : AssessmentModel, - exercise_model: ExerciseModel, + feedback_result: AssessmentModel, + exercise_model: ExerciseModel, manual_structured_grading_instructions: Optional[List[GradingCriterion]] = None ) -> List[Feedback]: - grading_instruction_ids = set( + grading_instruction_ids = { grading_instruction.id - for criterion in manual_structured_grading_instructions or [] + for criterion in (manual_structured_grading_instructions or []) for grading_instruction in criterion.structured_grading_instructions - ) + } feedbacks = [] for feedback in feedback_result.feedbacks: - # Each feedback has a grading_instruction_id. However we only want to have the grading_instruction_id in the final feedback that are associated with the manual structured grading instructions - grading_instruction_id = feedback.grading_instruction_id if feedback.grading_instruction_id in grading_instruction_ids else None - element_ids = [ - exercise_model.element_id_mapping[element] - for element in (feedback.element_names or []) - if element in exercise_model.element_id_mapping - ] + grading_instruction_id = ( + feedback.grading_instruction_id + if feedback.grading_instruction_id in grading_instruction_ids + else None + ) + + reference: Optional[str] = None + if feedback.element_name: + reference_id = exercise_model.element_id_mapping.get(feedback.element_name) + reference_type = exercise_model.id_type_mapping.get(reference_id) if reference_id else None + + if reference_type and reference_id: + reference = f"{reference_type}:{reference_id}" feedbacks.append(Feedback( exercise_id=exercise_model.exercise_id, submission_id=exercise_model.submission_id, title=feedback.title, description=feedback.description, - element_ids=element_ids, credits=feedback.credits, structured_grading_instruction_id=grading_instruction_id, meta={}, id=None, - is_graded=False + is_graded=False, + reference=reference, + element_ids=[reference] if reference else [] # Todo: Remove after adding migrations to athena )) return feedbacks \ No newline at end of file diff --git a/modules/modeling/module_modeling_llm/module_modeling_llm/utils/get_exercise_model.py b/modules/modeling/module_modeling_llm/module_modeling_llm/utils/get_exercise_model.py index a2457f1f4..3489be738 100644 --- a/modules/modeling/module_modeling_llm/module_modeling_llm/utils/get_exercise_model.py +++ b/modules/modeling/module_modeling_llm/module_modeling_llm/utils/get_exercise_model.py @@ -7,9 +7,9 @@ def get_exercise_model(exercise: Exercise, submission: Submission) -> ExerciseMo serialized_example_solution = None if exercise.example_solution: - serialized_example_solution, _, _ = ApollonJSONTransformer.transform_json(exercise.example_solution) + serialized_example_solution, _, _, _ = ApollonJSONTransformer.transform_json(exercise.example_solution) - transformed_submission, element_id_mapping, diagram_type = ApollonJSONTransformer.transform_json(submission.model) + transformed_submission, element_id_mapping, diagram_type, id_type_mapping = ApollonJSONTransformer.transform_json(submission.model) return ExerciseModel( submission_id=submission.id, @@ -22,6 +22,7 @@ def get_exercise_model(exercise: Exercise, submission: Submission) -> ExerciseMo submission_uml_type=diagram_type, transformed_example_solution=serialized_example_solution, element_id_mapping=element_id_mapping, + id_type_mapping=id_type_mapping )