[Feature:TAGrading] Backend for submission clustering#12886
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12886 +/- ##
============================================
- Coverage 21.38% 21.36% -0.03%
- Complexity 9946 9990 +44
============================================
Files 268 274 +6
Lines 37158 37300 +142
Branches 499 499
============================================
+ Hits 7948 7969 +21
- Misses 28715 28836 +121
Partials 495 495
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
williamjallen
left a comment
There was a problem hiding this comment.
Another round of comments. I'll have more once these are addressed. In addition to the inline comments, please:
- Change the PR title to reflect the fact this is not AI grading, but rather submission clustering
- Mark all comments you've addressed as "resolved" as you go so reviewers can easily see what you've responded to and what remains to be done.
I have just spoken with Barb about this issue, and she mentioned that students with late submissions should always be included in the clustering by default. However, if a student uploads a new submission after the initial clustering, that student should be removed from their cluster to prevent a version conflict in grading. She mentioned possibly adding a timestamp attribute to the clustering, so that clustered students submitting again after that timestamp can be removed. This feature would mostly be for students that got a due date exception and need to submit after clustering has already begun. |
|
Thanks for the schema document. For permanent archival purposes, can you please insert the example directly into the PR message notes rather than a external google drive document which may be deleted/unavailable. As I said in our meeting, the reason I was confused about the table purposes was that they are in a different order -- alphabetical by title -- in the .sql document. I understand them now. Name change suggestion: (this is not a required change, what do others @williamjallen, @NicholasCiuica think?) Also, I've chatted with other folks and I'm leaning towards using the vocabulary of "we perform a clustering" and then "the students are in groups" as a result. What do we think? And yes, to follow up on the discussion with @NicholasCiuica Note that a student's submission status being "late" or "too late/bad" should not be taken into account with the grouping. You should ignore that when doing the grouping. Similarly, I suggest we ignore whether a student is in the NULL registration section for now. Personally I do like using instructor/TA submissions as examples when we are grading or explaining how we will grade to our staff, so I would like any NULL section or teaching staff submissions to be treated like a regular student in the clustering. It's ok to delay the addition of the timestamp and other edits to a future PR. |
Table names should be all lower snake case IMO. I don't think it's a good idea to prefix these tables with "TA". If you think it's important to include "TA" in the names, what about prefixing with "ta_", e.g.,
I can't think of a reason why the ordering of the tables is particularly useful beyond this single PR where they're being added. I don't think the alphabetical order should influence our naming choices in any way.
I like that, assuming we don't want to use groups in a different way elsewhere in the future.
It seems reasonable to include them here IMO. |
|
The ordering of the tables also appears in SQL toolbox in the schema documentation, which for instructors who explore the database is the first exposure to a complicated structure. Naming these tables well (and coincidental alphabetic organization) is good. The capitalization & spacing was not my focus, sure yes, use "ta_grading_" as the prefix. |
I feel it would be better to stick with "clusters" as the name to make it very obvious that clusters are a result of the clustering action. Calling them groups sounds vague in comparison, and and to me it seems too similar to teams.
The following is what I imagine as the clearest names for the new tables added given the original wording, and they also flow nicely alphabetically. Grading Cluster Config -> ta_grading_clustering_configs |
Yeah I agree, I don't think it would be nice to call them "groups". I would say "cluster" is not that bad after all, also if you would like any change, changing it to "buckets" might be better than calling it "groups" would be better. But again I am not very supportive of this idea too. I think we should keep it simple , "TA create clusters", and as a result student submissions are "clustered" |
|
@NicholasCiuica @GarvitKhandelwal31 (We can always refactor the code/DB/UI later if group consensus changes.) |
NicholasCiuica
left a comment
There was a problem hiding this comment.
I've done a quick rereview. The code reorganization/name changes look good, and after some testing the new timestamp/active version logic looks good as well (I clustered, made another submission on a student, and saw that they were removed from their cluster).
The only potential issue I see now, which I don't know if I've seen before, is that if a GET clustering call is made with an invalid course code, a "success" response is returned, but whenever a POST create_clustering call is made with an invalid course code, an "error" response is returned, which I'd assume would be the behavior for both cases. Is it expected behavior that a clustering call on an invalid course should be a success? Please see the images below for the output I saw.
I just had a few more things to say- Well I would say this behaviour suits because the student submissions are immutable themselves, the version number is linked to a specific submission. Another edge case I was thinking of- |
You are correct, we may not strictly need the timestamp of clustering to be stored. But since we may eventually allow multiple clusterings of the same gradeable to be in use at the same time (TA A who is grading Problem 1 will want to cluster based on the student answers to problem 1, which TA B who is grading Problem 2 will want to use a different clustering). I think it might be good to know when a cluster was created. And eventually, who created it, and what they would like to name it...
This is the topic of "version conflict" So yes, manual grades using the one-at-a-time current interface and manual grades using the new cluster grading interface should both work the same if a student changes the version after any partial or complete manual grade has been recorded for their assignment. |
NicholasCiuica
left a comment
There was a problem hiding this comment.
Tested it once more and the error I found last review has been fixed.
### Why is this Change Important & Necessary?
This PR lays the architectural foundation, Doctrine entities, and
database schema support for the upcoming submission clustering project.
### What is the New Behavior?
1. **Database Schema**: Adds a new database migration to create three
tables (`grading_cluster_config`, `grading_cluster`, and
`grading_cluster_members`).
2. **Doctrine Entities**: Creates three Doctrine entities mapped to
those tables.
3. **API Controller**: Adds a new controller file to handle the
generation and fetching of clusters.
*Note: Currently, our design allows for global clustering only. If a TA
creates a clustering for student submissions using a particular
algorithm, it will be visible to all TAs. If they generate a new
clustering using another algorithm, the code automatically destroys any
previous clustering configuration for that gradeable and creates a new
one.*
### Database Schema Overview
For a deeper dive into the database schema, please refer to the below
explanation-
We use three tables to accomplish this:
**1. `grading_cluster_config`**
Stores the unique clustering configuration for a gradeable. Because
`g_id` is unique, we can only have one active clustering configuration
at a time per gradeable.
| id | g_id | algorithm |
|----|------|-----------|
| 1 | hw1 | algo1 |
| 2 | hw2_team_project | algo2 |
**2. `grading_cluster`**
Defines the individual clusters generated for a specific config. A TA
can assign or update a `cluster_name` to this cluster at any moment.
| id | config_id (FK) | cluster_name |
|----|----------------|--------------|
| 10 | 1 (links to hw1) | Cluster A |
| 11 | 1 (links to hw1) | Cluster B |
| 12 | 2 (links to hw2) | Group 1 |
| 13 | 2 (links to hw2) | Group 2 |
**3. `grading_cluster_members`**
Stores the members (users or teams) that belong to a cluster. It tracks
the student's active submission version at the time of clustering. It
also utilizes a `CHECK` constraint to ensure that either `user_id` or
`team_id` is filled out, but never both.
| id | cluster_id (FK)| user_id | team_id | active_version |
|----|----------------|----------|------------|----------------|
| 100| 10 (Cluster A) | student1 | NULL | 1 |
| 101| 10 (Cluster A) | student2 | NULL | 2 |
| 102| 11 (Cluster B) | student3 | NULL | 3 |
| 104| 12 (Group 1) | NULL | team_alpha | 1 |
| 105| 13 (Group 2) | NULL | team_beta | 2 |
### What steps should a reviewer take to reproduce or test the bug or
new feature?
Since there is no frontend UI yet, you can test the backend pipeline
using the browser's developer console.
1. Run `submitty_install` in vagrant (to apply the migrations).
2. Login as an instructor and navigate to any gradeable page.
3. Open the browser developer console.
**To create clusters:**
let formData = new FormData();
formData.append('csrf_token', csrfToken);
formData.append('algorithm', 'dummy_split');
fetch(buildCourseUrl(['gradeable', '<GRADEABLE_ID>',
'create_clustering']), {
method: 'POST',
body: formData
})
.then(res => res.json())
.then(console.log);
To fetch them-
fetch(buildCourseUrl(['gradeable', '<GRADEABLE_ID>', 'clustering']))
.then(res => res.json())
.then(console.log);
Over here please verify that the students with registration section NULL
and have late submissions are also included.
### Automated Testing & Documentation
Added tests to verify the methods in the controller.
### Other information
Migrations are included, so please ensure you run submitty_install
before reviewing or testing the PR locally.
* main: (78 commits) [Bugfix:TAGrading] Auto-open single file (#12625) [Feature:InstructorUI] Add sortable columns to manage students (#12957) [Dependency] Bump php-ds/php-ds from 1.7.0 to 2.0.1 in /site (#12827) [Refactor:TAGrading] Events Infrastructure & StatusBanner (#12941) [Dependency] Bump sqlalchemy from 2.0.48 to 2.0.51 in /.setup/pip (#12884) [Bugfix:Developer] trust hashicorp for vagrant install (#12968) [Feature:TAGrading] Backend for submission clustering (#12886) [Bugfix:InstructorUI] Missing Subsection in the Edit User form (#12955) [Bugfix:Submission] Warning Banners Accessibility Fix (#12956) [Bugfix:System] Preserve signup fields (#12939) [Bugfix:InstructorUI] Fix Csv Download of Subsections (#12954) [Bugfix:InstructorUI] Fix Manage Students Toggle Columns (#12953) [Feature:TAGrading] Persistent Auto-Open (#12931) [Bugfix:Submission] Dot File Upload (#12924) [Refactor:System] Declutter workers (#12815) [Bugfix:Developer] save/restore autograding_containers.json (#12952) [Bugfix:InstructorUI] Create Course Validation (#12949) [Testing:Notifications] Fix grade_inquiries.spec.js (#12950) [UI/UX:System] Add fullscreen button vue component (#12936) [Bugfix:TAGrading] Student Name Resize (#12946) ... # Conflicts: # .pylintrc
### Why is this Change Important & Necessary? This PR adds a daemon job to support asynchronous submission clustering. ### What is the New Behavior? Now the submission clustering algorithms run asynchronously. In this PR we have also removed some the files that would not be needed now that was introduced in #12886. ### What steps should a reviewer take to reproduce or test the bug or new feature? 1. Run submitty_install 2. Run php unit test (in site folder)- vendor/bin/phpunit tests/app/controllers/grading/GradingClusterControllerTester.php 3. Run python unit test - cd /usr/local/submitty/GIT_CHECKOUT/Submitty/sbin/submitty_daemon_jobs python3 -m unittest tests/test_grading_clustering.py Test if backend is fine- 1) First inside site folder to see the logs- tail -f /var/local/submitty/logs/daemon_job_queue/$(date +%Y%m%d).txt 2) Go to any gradeable, in the browser console, hit a post request- let formData = new FormData(); formData.append('csrf_token', csrfToken); formData.append('algorithm', 'dummy_split'); fetch(buildCourseUrl(['gradeable', '<GRADEABLE_ID>', 'create_clustering']), { method: 'POST', body: formData }) .then(res => res.json()) .then(console.log); Please check the daemon log- <img width="1435" height="142" alt="image" src="https://github.com/user-attachments/assets/85cbaa8f-3a79-4add-8df9-83829fb7d71d" /> 3) Make a get request to verify the clusters formed- fetch(buildCourseUrl(['gradeable', '<GRADEABLE_ID>', 'clustering'])) .then(res => res.json()) .then(console.log); ### Automated Testing & Documentation The core Python clustering logic is covered by unit tests in `sbin/submitty_daemon_jobs/tests/test_grading_clustering.py`. The PHP Controller is covered by `tests/app/controllers/grading/GradingClusterControllerTester.php`. ### Other information Please run submitty_install before reviewing.
### Why is this Change Important & Necessary? This PR adds a daemon job to support asynchronous submission clustering. ### What is the New Behavior? Now the submission clustering algorithms run asynchronously. In this PR we have also removed some the files that would not be needed now that was introduced in Submitty#12886. ### What steps should a reviewer take to reproduce or test the bug or new feature? 1. Run submitty_install 2. Run php unit test (in site folder)- vendor/bin/phpunit tests/app/controllers/grading/GradingClusterControllerTester.php 3. Run python unit test - cd /usr/local/submitty/GIT_CHECKOUT/Submitty/sbin/submitty_daemon_jobs python3 -m unittest tests/test_grading_clustering.py Test if backend is fine- 1) First inside site folder to see the logs- tail -f /var/local/submitty/logs/daemon_job_queue/$(date +%Y%m%d).txt 2) Go to any gradeable, in the browser console, hit a post request- let formData = new FormData(); formData.append('csrf_token', csrfToken); formData.append('algorithm', 'dummy_split'); fetch(buildCourseUrl(['gradeable', '<GRADEABLE_ID>', 'create_clustering']), { method: 'POST', body: formData }) .then(res => res.json()) .then(console.log); Please check the daemon log- <img width="1435" height="142" alt="image" src="https://github.com/user-attachments/assets/85cbaa8f-3a79-4add-8df9-83829fb7d71d" /> 3) Make a get request to verify the clusters formed- fetch(buildCourseUrl(['gradeable', '<GRADEABLE_ID>', 'clustering'])) .then(res => res.json()) .then(console.log); ### Automated Testing & Documentation The core Python clustering logic is covered by unit tests in `sbin/submitty_daemon_jobs/tests/test_grading_clustering.py`. The PHP Controller is covered by `tests/app/controllers/grading/GradingClusterControllerTester.php`. ### Other information Please run submitty_install before reviewing.
Why is this Change Important & Necessary?
This PR lays the architectural foundation, Doctrine entities, and database schema support for the upcoming submission clustering project.
What is the New Behavior?
grading_cluster_config,grading_cluster, andgrading_cluster_members).Note: Currently, our design allows for global clustering only. If a TA creates a clustering for student submissions using a particular algorithm, it will be visible to all TAs. If they generate a new clustering using another algorithm, the code automatically destroys any previous clustering configuration for that gradeable and creates a new one.
Database Schema Overview
For a deeper dive into the database schema, please refer to the below explanation-
We use three tables to accomplish this:
1.
grading_cluster_configStores the unique clustering configuration for a gradeable. Because
g_idis unique, we can only have one active clustering configuration at a time per gradeable.2.
grading_clusterDefines the individual clusters generated for a specific config. A TA can assign or update a
cluster_nameto this cluster at any moment.3.
grading_cluster_membersStores the members (users or teams) that belong to a cluster. It tracks the student's active submission version at the time of clustering. It also utilizes a
CHECKconstraint to ensure that eitheruser_idorteam_idis filled out, but never both.What steps should a reviewer take to reproduce or test the bug or new feature?
Since there is no frontend UI yet, you can test the backend pipeline using the browser's developer console.
submitty_installin vagrant (to apply the migrations).To create clusters:
let formData = new FormData();
formData.append('csrf_token', csrfToken);
formData.append('algorithm', 'dummy_split');
fetch(buildCourseUrl(['gradeable', '<GRADEABLE_ID>', 'create_clustering']), {
method: 'POST',
body: formData
})
.then(res => res.json())
.then(console.log);
To fetch them-
fetch(buildCourseUrl(['gradeable', '<GRADEABLE_ID>', 'clustering']))
.then(res => res.json())
.then(console.log);
Over here please verify that the students with registration section NULL and have late submissions are also included.
Automated Testing & Documentation
Added tests to verify the methods in the controller.
Other information
Migrations are included, so please ensure you run submitty_install before reviewing or testing the PR locally.