Skip to content
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

Development: Support Pyris V2 #8286

Merged
merged 251 commits into from
May 10, 2024
Merged

Development: Support Pyris V2 #8286

merged 251 commits into from
May 10, 2024

Conversation

Hialus
Copy link
Member

@Hialus Hialus commented Apr 2, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
  • I documented the Java code using JavaDoc style.

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data.
  • I strictly followed the client coding and design guidelines.
  • Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I added authorities to all new routes and checked the course groups for displaying navigation elements (links, buttons).
  • I documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

Motivation and Context

The Iris WG has come to the conclusion that PyrisV1, which was based on guidance, is no longer the best way forward for Iris. So we decided that we should replace it with a new version called PyrisV2.
For this the entire Pyris Connector and the way responses are handled needs to be replaced in Artemis. This PR implements the first steps for proper PyrisV2 support.

Description

There are numerous changes that were needed:

  • Entirely new PyrisV2 API that has to be supported with full DTO coverage
  • Responses of Pyris now come in asynchronously via status updates
    • A list of active Pyris jobs in Hazelcast
    • Each job with a custom access token for security and easy identification
  • New improved way to show the student what the progress of the pipeline is
  • New mocking system for the server tests

The client will get some further changes in a future PR by @bassner.
The other features of Iris will be reenabled in future PRs.

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 1 Programming exercise with the Iris Tutor Chat enabled
  1. Log in to Artemis
  2. Go to the exercise student page
  3. Send a chat message -> You get status updates and a response

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked



Review Progress

Performance Review

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Coverage

Client

Class/File Line Coverage Confirmation (assert/expect)
iris-settings-update.component.ts 82.66%

Server

Class/File Line Coverage Confirmation (assert/expect)
RestTemplateConfiguration.java 95%
IrisSession.java 100%
ProgrammingSubmissionRepository.java 62%
PyrisConnectorException.java 100%
PyrisConnectorService.java 79%
PyrisDTOService.java 50%
PyrisHealthIndicator.java 76%
PyrisJobService.java 94%
PyrisPipelineService.java 100%
PyrisStatusUpdateService.java 83%
PyrisJob.java 62%
TutorChatJob.java 42%
IrisChatSessionService.java 87%
IrisCodeEditorSessionService.java 8% ❌ (Will be removed in another PR)
IrisCompetencyGenerationSessionService.java 14% ❌ (Will be readded in a follow up)
IrisHestiaSessionService.java 24% ❌ (Will be readded in a follow up)
IrisSettingsService.java 79%
IrisSubSettingsService.java 89%
IrisChatWebsocketService.java 69%
IrisChatSessionResource.java 92%
IrisCodeEditorSessionResource.java 92%
IrisExerciseChatBasedSessionResource.java 85%
IrisSettingsResource.java 96%
PyrisStatusUpdateResource.java 70%
ProgrammingExerciseExportImportResource.java 92%

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Out of diff range and nitpick comments (1)
src/main/java/de/tum/in/www1/artemis/domain/iris/settings/IrisModelListConverter.java (1)

Line range hint 24-30: The method convertToEntityAttribute effectively splits the string and converts it back into a SortedSet. Consider adding error handling for potential exceptions from malformed strings.

    public SortedSet<String> convertToEntityAttribute(String value) {
        var treeSet = new TreeSet<String>(Comparator.naturalOrder());
        try {
            if (value != null) {
                treeSet.addAll(Set.of(value.split(",")));
            }
        } catch (Exception e) {
            // Log and handle the exception appropriately
            // Consider rethrowing as a specific checked exception if necessary
        }
        return treeSet;
    }

coderabbitai[bot]
coderabbitai bot previously approved these changes May 6, 2024
coderabbitai[bot]
coderabbitai bot previously approved these changes May 6, 2024
@krusche krusche changed the title Iris: Support PyrisV2 Development: Support Pyris V2 May 10, 2024
Copy link
Member

@krusche krusche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my feedback. I only have some minor questions / comments left

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Out of diff range and nitpick comments (1)
src/main/java/de/tum/in/www1/artemis/service/iris/session/IrisChatSessionService.java (1)

20-28: Consider organizing imports to improve readability and maintainability.

@krusche krusche added this to the 7.0.5 milestone May 10, 2024
coderabbitai[bot]
coderabbitai bot previously approved these changes May 10, 2024
coderabbitai[bot]
coderabbitai bot previously approved these changes May 10, 2024
@krusche krusche merged commit ec6c511 into develop May 10, 2024
19 of 23 checks passed
@krusche krusche deleted the feature/iris/pyrisV2 branch May 10, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) component:Iris ready for review server Pull requests that update Java code. (Added Automatically!) tests too-long-open !!! This is an antipattern, we should aim for small PRs that are only open for a short time !!!
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet