-
Notifications
You must be signed in to change notification settings - Fork 313
Communication
: Add first Iris Tutor Suggestion Interface
#10666
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
base: develop
Are you sure you want to change the base?
Changes from 46 commits
4823709
2c2409f
632601c
2f6eeae
1100dd2
8a8d6b1
136c1e0
99dc452
efb15ce
300342b
83147a6
20cd8c0
2850d26
8281751
f5e1b07
a1eb62f
5c5c7cd
7e1902f
e297361
90284d0
092ac38
52aa20a
8e63731
6b49658
37b6a87
74ab340
5510a2b
26cd047
eef8e9e
8911a3e
61af444
5d0c637
839523a
98ed716
d9a27c3
47395f1
04fe012
5c5fc87
b7a868e
332b56d
402e034
8168014
9409180
3b5200f
1290783
f9228a8
2900c68
3ef3a76
ae1738f
a6d9eaf
2b1794b
4527e7d
8b20bad
21a5769
011b374
6e91018
73d8df0
4d03481
fe16baf
0d54448
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
package de.tum.cit.aet.artemis.iris.domain.session; | ||
|
||
import java.util.Optional; | ||
|
||
import jakarta.persistence.DiscriminatorValue; | ||
import jakarta.persistence.Entity; | ||
import jakarta.persistence.ManyToOne; | ||
|
||
import com.fasterxml.jackson.annotation.JsonIgnore; | ||
import com.fasterxml.jackson.annotation.JsonInclude; | ||
|
||
import de.tum.cit.aet.artemis.communication.domain.Post; | ||
import de.tum.cit.aet.artemis.core.domain.User; | ||
|
||
/** | ||
* An IrisTutorSuggestionSession represents a conversation between a user and an LLM in the context of a tutor suggestion. | ||
* This is used for tutors receiving assistance from Iris for answering student questions. | ||
*/ | ||
@Entity | ||
@DiscriminatorValue("TUTOR_SUGGESTION") | ||
@JsonInclude(JsonInclude.Include.NON_EMPTY) | ||
public class IrisTutorSuggestionSession extends IrisChatSession { | ||
|
||
@ManyToOne | ||
@JsonIgnore | ||
private Post post; | ||
|
||
public IrisTutorSuggestionSession(Post post, User user) { | ||
super(user); | ||
this.post = post; | ||
} | ||
|
||
public IrisTutorSuggestionSession() { | ||
} | ||
|
||
public Post getPost() { | ||
return post; | ||
} | ||
|
||
public void setPost(Post post) { | ||
this.post = post; | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return "IrisTutorSuggestionSession{" + "user=" + Optional.ofNullable(getUser()).map(User::getLogin).orElse("null") + "," + "post=" + post + '}'; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,6 +52,10 @@ public class IrisCourseSettings extends IrisSettings { | |
@JoinColumn(name = "iris_competency_generation_settings_id") | ||
private IrisCompetencyGenerationSubSettings irisCompetencyGenerationSettings; | ||
|
||
@OneToOne(cascade = CascadeType.ALL, orphanRemoval = true, fetch = FetchType.EAGER) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The many eager fetches here can become a problem with Hibernate. We definitely need to change this in the future, this has become way too complex when you think about that a few boolean values are stored. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Hialus I suggest we talk about this asap |
||
@JoinColumn(name = "iris_tutor_suggestion_settings_id") | ||
private IrisTutorSuggestionSubSettings irisTutorSuggestionSettings; | ||
|
||
public Course getCourse() { | ||
return course; | ||
} | ||
|
@@ -129,4 +133,14 @@ public IrisFaqIngestionSubSettings getIrisFaqIngestionSettings() { | |
public void setIrisFaqIngestionSettings(IrisFaqIngestionSubSettings irisFaqIngestionSubSettings) { | ||
this.irisFaqIngestionSettings = irisFaqIngestionSubSettings; | ||
} | ||
|
||
@Override | ||
public IrisTutorSuggestionSubSettings getIrisTutorSuggestionSettings() { | ||
return irisTutorSuggestionSettings; | ||
} | ||
|
||
@Override | ||
public void setIrisTutorSuggestionSettings(IrisTutorSuggestionSubSettings irisTutorSuggestionSubSettings) { | ||
this.irisTutorSuggestionSettings = irisTutorSuggestionSubSettings; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
package de.tum.cit.aet.artemis.iris.domain.settings; | ||
|
||
import jakarta.persistence.DiscriminatorValue; | ||
import jakarta.persistence.Entity; | ||
|
||
import com.fasterxml.jackson.annotation.JsonInclude; | ||
|
||
/** | ||
* An {@link IrisSubSettings} implementation for the settings for tutor suggestions. | ||
*/ | ||
@Entity | ||
@DiscriminatorValue("TUTOR_SUGGESTION") | ||
@JsonInclude(JsonInclude.Include.NON_EMPTY) | ||
public class IrisTutorSuggestionSubSettings extends IrisSubSettings { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need so many iris settings? It becomes more and more complicated. I do not really think that instructors want to specify so many details There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can discuss how we could connect the feature to other settings of Iris, but being able to disable it should be possible in my opinion There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is that we have so many different settings that can be enabled and disabled. I don't think that instructors need so many options. This simply leads to the fact, that the feature is not used at all, because it is disabled by default, no instructor knows about the feature and even if they would know, they would not find how to enable it. We have the lecture "ingestion" feature for more than 6 months now, however no one has used it so far :-( Also this means that everytime, we need to load settings if something is enabled or not in the client user interface. This also leads to issues. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe there should be a simple switch for instructors to either enable IRIS completely or disable it completely. We could have an intermediate state where the "most important" and "most stable" features are active, but we really do not need so many settings. Think about other tools that you use. Are you enabled or disabling all features with lots of different settings or do you trust the software provider that it works? Using the features is still voluntary (and requries opt-in), no one is forced. |
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package de.tum.cit.aet.artemis.iris.dto; | ||
|
||
import java.util.Set; | ||
|
||
import jakarta.annotation.Nullable; | ||
|
||
import com.fasterxml.jackson.annotation.JsonInclude; | ||
|
||
/** | ||
* Data transfer object for the IrisCombinedTutorSuggestionSubSettings. | ||
* | ||
* @param enabled true if settings are enabled | ||
* @param allowedVariants a set of allowed variants | ||
* @param selectedVariant the selected variant | ||
*/ | ||
@JsonInclude(JsonInclude.Include.NON_EMPTY) | ||
public record IrisCombinedTutorSuggestionSubSettingsDTO(boolean enabled, @Nullable Set<String> allowedVariants, @Nullable String selectedVariant) { | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
package de.tum.cit.aet.artemis.iris.repository; | ||
|
||
import static de.tum.cit.aet.artemis.core.config.Constants.PROFILE_IRIS; | ||
import static org.springframework.data.jpa.repository.EntityGraph.EntityGraphType.LOAD; | ||
|
||
import java.util.Collections; | ||
import java.util.List; | ||
|
||
import org.springframework.context.annotation.Profile; | ||
import org.springframework.data.domain.Pageable; | ||
import org.springframework.data.jpa.repository.EntityGraph; | ||
import org.springframework.stereotype.Repository; | ||
|
||
import de.tum.cit.aet.artemis.core.domain.DomainObject; | ||
import de.tum.cit.aet.artemis.core.repository.base.ArtemisJpaRepository; | ||
import de.tum.cit.aet.artemis.iris.domain.session.IrisTutorSuggestionSession; | ||
|
||
@Repository | ||
@Profile(PROFILE_IRIS) | ||
public interface IrisTutorSuggestionSessionRepository extends ArtemisJpaRepository<IrisTutorSuggestionSession, Long> { | ||
|
||
List<IrisTutorSuggestionSession> findByPostIdAndUserIdOrderByCreationDateDesc(Long postId, Long userId, Pageable pageable); | ||
|
||
@EntityGraph(type = LOAD, attributePaths = "messages") | ||
List<IrisTutorSuggestionSession> findSessionsWithMessagesByIdIn(List<Long> ids); | ||
|
||
/** | ||
* Finds the latest sessions for the given post and user with messages. | ||
* | ||
* @param postId the id of the post | ||
* @param userId the id of the user | ||
* @param pageable the pageable to use for the query | ||
* @return the latest sessions for the given post and user with messages | ||
*/ | ||
default List<IrisTutorSuggestionSession> findLatestSessionsByPostIdAndUserIdWithMessages(Long postId, Long userId, Pageable pageable) { | ||
List<Long> ids = findByPostIdAndUserIdOrderByCreationDateDesc(postId, userId, pageable).stream().map(DomainObject::getId).toList(); | ||
|
||
if (ids.isEmpty()) { | ||
return Collections.emptyList(); | ||
} | ||
|
||
return findSessionsWithMessagesByIdIn(ids); | ||
} | ||
} |
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.
Keep in mind that this will fetch a lot of data due to Hibernate. Do you really need all the data behind it or is the post_id enough, especially if you use json ignore here?
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.
I need most of the information the Post contains. I could only save the postId and fetch the Post via the repository. As every IrisSession saves its reference that way, I wanted to be consistent with the implementation.
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.
I would argue to change this in other places as well, otherwise we might get severe performance issues. Do you really always need the post information when you load the session? Or is there one specific case where you need this?