Immediate automated feedback and grades on course dashboard and beginning of automated denis checks by theyoyojo · Pull Request #256 · underground-software/singularity

theyoyojo · 2025-05-22T02:26:30Z

Bring back automated feedback

Before due date: show accepted/issues/rejected

After due date: show full status string generated by mailman/patchset.py

Show final submission, review, and total scores on dashboard based on git notes

Framework for automated tests by denis on initial and final tag promotion and a couple of checks

Fixes #255
Fixes #258
Fixes #259
Fixes #260
Fixes #232
Fixes #206
Fixes #111
Fixes #110
Fixes #200
Fixes #262
Fixes #257
Fixes #207
Fixes #202
Fixes #145
Fixes #109

For the change that adds the warning msg when denis is dirty: it could print the time it was dirtied using the /tmp/dirty modification time. Not sure how useful it would be but it would be trivial to implement

Patchsets modifying files outside of student directory are detected by scanning the signed off by line and are rejected if:

there is no signed off by line
the patches modify anything outside of submissions/foo where the signed off by line looks like Signed-off-by:
foo-bar <foo@$COURSE_DOMAIN>

charliemirabile

Nice. A few minor nits, but overall simple and pretty clean and definitely moving in the right direction.

charliemirabile · 2025-05-22T15:30:27Z

+#!/bin/bash
+
+set -exuo pipefail
+
+SCRIPT_DIR=$(dirname $0)
+
+cd $SCRIPT_DIR
+
+get_git_port() { podman port singularity_git_1 | awk -F':' '{ print $2 }' ; } ;
+
+# nuke grading repo inside container
+podman-compose exec git ash -c 'cd /var/lib/git/grading.git && for t in $(git tag); do git tag -d $t; done'
+
+# setup submissions repo
+rm -rf repos/submissions
+git/admin.sh submissions "course submissions repository"
+pushd repos
+git init --bare submissions
+git init submissions_init
+pushd submissions_init
+echo "# submissions" > README.md
+git add README.md
+git -c user.name=singularity -c user.email=singularity@singularity commit -m 'init submissions repo'
+git push ../submissions master
+popd
+rm -rf submissions_init
+pushd submissions
+git push --mirror http://localhost:$(get_git_port)/cgi-bin/git-receive-pack/submissions
+popd
+popd
+
+# create bob user or change password to builder
+orbit/warpdrive.sh -u bob -p builder -n || orbit/warpdrive.sh -u bob -p builder -m
+
+# create or recreate setup assignment
+if denis/configure.sh dump | grep -q "^setup:"; then
+	denis/configure.sh remove -a setup
+fi
+denis/configure.sh create -a setup -i $(date -d "10 secs" +%s) -p $(date -d "15 secs" +%s) -f $(date -d "20 secs" +%s)
+denis/configure.sh reload
+
+WORKDIR=$(mktemp -d)
+
+# setup assignment environment
+pushd $WORKDIR
+
+mkdir certs
+pushd certs
+podman volume export singularity_ssl-certs > certs.tar
+tar xf certs.tar
+popd
+
+git clone http://localhost:$(get_git_port)/submissions
+mkdir submissions/bob
+pushd submissions/bob
+git config user.name bob
+git config user.email bob@localhost.localdomain
+git config sendemail.smtpUser bob
+git config sendemail.smtpPass builder
+git config sendemail.smtpserver localhost.localdomain
+git config sendemail.smtpserverport 465
+git config sendemail.smtpencryption ssl
+mkdir good corrupt whitespace nocover nocover-corrupt
+
+# create patchsets
+
+# good patchset
+pushd good
+
+echo "abc" > work
+git add work
+git commit -sm 'add abc to work'
+
+echo "def" >> work
+git add work
+git commit -sm 'add def to work'
+
+git format-patch --rfc --cover-letter -v1 -2
+sed -i 's/\*\*\* SUBJECT HERE \*\*\*/Good patchset/g' v1-0000-cover-letter.patch
+sed -i 's/\*\*\* BLURB HERE \*\*\*/Good patchset/g' v1-0000-cover-letter.patch
+
+git send-email --confirm=never --smtp-ssl-cert-path=$WORKDIR/certs/fullchain.pem --to setup@localhost.localdomain *.patch
+popd
+
+# corrupt patchset
+pushd corrupt
+
+echo "abc" > work
+git add work
+git commit -sm 'add abc to work'
+
+echo "def" >> work
+git add work
+git commit -sm 'add def to work'
+
+echo "ghi" >> work
+git add work
+git commit -sm 'add ghi to work'
+
+git format-patch --rfc --cover-letter -v1 -2
+sed -i 's/\*\*\* SUBJECT HERE \*\*\*/Corrupt patchset/g' v1-0000-cover-letter.patch
+sed -i 's/\*\*\* BLURB HERE \*\*\*/Corrupt patchset/g' v1-0000-cover-letter.patch
+
+git send-email --confirm=never --smtp-ssl-cert-path=$WORKDIR/certs/fullchain.pem --to setup@localhost.localdomain *.patch
+popd
+
+# patchset with whitespace errors
+pushd whitespace
+
+echo "abc" > work
+git add work
+git commit -sm 'add abc to work'
+
+echo "def	" >> work
+git add work
+git commit -sm 'add def to work'
+
+echo "ghi	" >> work
+git add work
+git commit -sm 'add ghi to work'
+
+git format-patch --rfc --cover-letter -v1 -3
+sed -i 's/\*\*\* SUBJECT HERE \*\*\*/Patchset with whitespace errors/g' v1-0000-cover-letter.patch
+sed -i 's/\*\*\* BLURB HERE \*\*\*/Patchset with whitespace errors/g' v1-0000-cover-letter.patch
+
+git send-email --confirm=never --smtp-ssl-cert-path=$WORKDIR/certs/fullchain.pem --to setup@localhost.localdomain *.patch
+popd
+
+# patchset with no cover letter
+pushd nocover
+
+echo "abc" > work
+git add work
+git commit -sm 'add abc to work'
+
+echo "def" >> work
+git add work
+git commit -sm 'add def to work'
+
+git format-patch --rfc -v1 -2
+
+git send-email --confirm=never --smtp-ssl-cert-path=$WORKDIR/certs/fullchain.pem --to setup@localhost.localdomain *.patch
+popd
+
+# patchset with no cover letter and corrupt first patch
+pushd nocover-corrupt
+
+echo "abc" > work
+git add work
+git commit -sm 'add abc to work'
+
+echo "def" >> work
+git add work
+git commit -sm 'add def to work'
+
+echo "ghi" >> work
+git add work
+git commit -sm 'add ghi to work'
+
+git format-patch --rfc -v1 -2
+
+git send-email --confirm=never --smtp-ssl-cert-path=$WORKDIR/certs/fullchain.pem --to setup@localhost.localdomain *.patch
+popd
+
+# end of patchset sending
+popd
+
+sleep 1
+
+# investigate grading repo
+git clone http://localhost:$(get_git_port)/grading.git
+pushd grading
+git fetch --tag
+git tag
+
+
+STATUSES=(
+'patchset applies.'
+'patch 1 failed to apply!'
+'whitespace error patch(es) 2,3?'
+'missing cover letter!'
+'missing cover letter and first patch failed to apply!')
+
+# submitted patchses should have statuses in this order
+# assumption: first ID tag is first patch submitted by this script and IDs increase monotonically
+i=0
+for t in $(git tag); do
+	git show -s --oneline $t | grep -q "${STATUSES[$i]}"
+	i=$((i+1))
+done
+
+popd
+
+echo "wait for initial submission deadline (10 secs)"
+sleep 10
+
+# make final submission
+pushd submissions/bob
+
+# good final patchset
+mkdir good-final
+pushd good-final
+
+echo "abc" > work
+git add work
+git commit -sm 'add abc to work'
+
+echo "def" >> work
+git add work
+git commit -sm 'add def to work'
+
+git format-patch --cover-letter -v2 -2
+sed -i 's/\*\*\* SUBJECT HERE \*\*\*/Good patchset final/g' v2-0000-cover-letter.patch
+sed -i 's/\*\*\* BLURB HERE \*\*\*/Good patchset final/g' v2-0000-cover-letter.patch
+
+git send-email --confirm=never --smtp-ssl-cert-path=$WORKDIR/certs/fullchain.pem --to setup@localhost.localdomain *.patch
+popd
+
+# end final submission
+popd
+
+echo "wait for final submission deadline (10 secs)"
+sleep 10
+
+pushd grading
+git fetch --tag
+
+echo $WORKDIR


maybe this goes in circumference or wherever we had that other script with users bib bob beb and bab.

sure though we should put something that does this sort of thing in the main tests

this is just an initial draft though

charliemirabile

some more nits

charliemirabile · 2025-05-22T19:48:55Z

-            <th>Comments</th>
-            <td colspan="3">-</td>
+            <th>Automated Feedback</th>
+            <td colspan="3">{self.get_automated_feedback('final')}</td>


I think that this should maybe remain unchanged. This is where the official human feedback should get slotted in. Instead of reading from the status, if the submission is an insta zero then I think that denis should promote the status to a git note providing feedback (which is how it would end up showing up here) and also add a tag officially giving them a zero. I am not totally sure where to put the info before the due date. Maybe the box below automated feedback can do double duty providing the auto feedback for both initial and final depending on the time?

We can also just add another line and have both "comments' and "automated feedback"

charliemirabile · 2025-05-22T19:51:57Z



-def tag_and_push(repo, tag_name):
+def tag_and_push(repo, tag_name, msg=None):


Not sure this needs a default argument

We don't use it in apply_peer_review

charliemirabile · 2025-05-22T19:53:02Z

          {self.gradeable_row(self.peer1 + ' Peer Review', self.review1, '-') if self.peer1 else ''}
          {self.gradeable_row(self.peer2 + ' Peer Review', self.review2, '-') if self.peer2 else ''}


I think you could just generalize the above function to check for notes on the tags for the peer review too. The workflow would be the same.

in progress

Verify that the diffstat in a submitted cover letter matches the diffstat calculated with `git diff --stat --summary $(git rev-list --max-parents=0 <tag>)..<tag>` Fixes #111 Signed-off-by: Joel Savitz <joel@underground.software>

compare DCO in each patch and cover letter to one created by combining the known username and hostname. Fixes #110 Signed-off-by: Joel Savitz <joel@underground.software>

Check RFC/lack thereof, PATH, vN consistency, and correct indexes Fixes: #262 Signed-off-by: Joel Savitz <joel@underground.software>

Grader can write to this section via the feedback ref on the relevant final submission tag, e.g.: $ git notes --ref=feedback add setup_final_bob -m 'Feedback message' Signed-off-by: Joel Savitz <joel@underground.software>

When the assignment database is altered, the due date waiters must be restarted in order for the new due dates to be operative using: $ denis/configure.sh reload Currently there is no way to know that one has forgotten to run this command. Add a warning message to the output of: $ denis/configure.sh dump to indicate when one has altered the database but not yet operationalized their intentions. Fixes #257 Signed-off-by: Joel Savitz <joel@underground.software>

Any patchset containing a hunk modifying a file outside of a directory named after their username at the top level of the submissions repository, as specified by the DCO in that particular patch, is rejected immediately upon submission. If no additional patchset is submitted before a deadline, the student will automatically receive a zero. Additionally, patches lacking a DCO at submission time are rejected. Fixes #145 Signed-off-by: Joel Savitz <joel@underground.software>

Run the peer review tags from denis/peer_review.py through the same run_automated_checks() as the initial and final tags, but skip the checks that don't make sense. This opens up the door for checking peer review specific requirements like the Acked-by and Nacked-by. Signed-off-by: Joel Savitz <joel@underground.software>

theyoyojo · 2025-06-02T02:12:59Z

I think I broke the diffstat check at some point (or maybe it was always broken) so I need to check that out

Add support for checking whether changes made in each patch of a patchset conform to the expected set of changes. Only simple changes are supported, i.e. either required file creation or required file modification, both with the path to the file precisely specified. Mailman also requires the patchset to contain the expected number of patches To create a rubric, make commits on some repo, perhaps submissions, that make the expected changes for the expected number of patches N and run: $ $SINGULARITY_DIR/create_rubric.py -n $N > rubric To generate a file that can be passed to denis via: $ denis/configure.sh -r /path/to/rubric <...other args...> The rubric for an assignment can be viewed with denis/configure.sh dump. The rubric is enforced at submissison time, i.e. mailman rejects nonconforming patchsets immediately, marks them as corrupt, and creates a submission ID tag that contains the patch emails as empty patches. If no follow up submission is made, the student will receive a 0 for that component of the assignment. Fixes #109 Signed-off-by: Joel Savitz <joel@underground.software>

Setup the submissions repo and go through the entire patchset submission process, exercising all return paths of mailman/patchset.py Includes intitial, peer review, and final submissions Signed-off-by: Joel Savitz <joel@underground.software>

This script the project in a more integration-test-like manner. Signed-off-by: Joel Savitz <joel@underground.software>

test-sub.sh is good for interactive testing, but lacks validation when automated. Add test-sub-check.sh to run some basic checks that make sure test-sub.sh ran smoothly. Signed-off-by: Joel Savitz <joel@underground.software>

We use podman around these parts Signed-off-by: Joel Savitz <joel@underground.software>

test.sh and test-sub-check.sh now source test-lib script-lint.sh modified to follow source commands for those two files. Signed-off-by: Joel Savitz <joel@underground.software>

Add test-sub2.sh which tests out 12 invalid and 1 valid submission on a rubric for an example coding assignment and validate the resulting status values added to the email ID tags in the grading repo. Simplify the new tests by abstracting the setup section and repetitive commands, enabling quicker development of shorter future testing scripts the same flavor as the rest of test-sub*.sh. This is important as we want to make sure our automated grading features actually do what we think they will do to the fullest extent possible before this code goes into production with real students. Signed-off-by: Joel Savitz <joel@underground.software>

Submissions.status has the same field name and refers to a different type of message so rename this one to better reflect its content. Also update some variable names to reflect this change. Signed-off-by: Joel Savitz <joel@underground.software>

If a student makes an inital submission but not a final submission, they should automatically receive a zero for the assignment. Signed-off-by: Joel Savitz <joel@underground.software>

theyoyojo changed the title ~~Mailman enhanced~~ Immediate automated feedback on dashboard May 22, 2025

theyoyojo force-pushed the mailman_enhanced branch from c3ebe61 to dea0099 Compare May 22, 2025 15:24

charliemirabile requested changes May 22, 2025

View reviewed changes

theyoyojo force-pushed the mailman_enhanced branch 5 times, most recently from e6d3889 to b5ee886 Compare May 22, 2025 18:27

charliemirabile requested changes May 22, 2025

View reviewed changes

theyoyojo force-pushed the mailman_enhanced branch 3 times, most recently from 63bbfa1 to 38e1d0c Compare May 22, 2025 20:19

theyoyojo changed the title ~~Immediate automated feedback on dashboard~~ Immediate automated feedback and grades on course dashboard May 22, 2025

theyoyojo force-pushed the mailman_enhanced branch 5 times, most recently from 73e53b8 to 2fc4fac Compare May 23, 2025 04:34

theyoyojo changed the title ~~Immediate automated feedback and grades on course dashboard~~ Immediate automated feedback and grades on course dashboard and beginning of automated denis checks May 23, 2025

theyoyojo force-pushed the mailman_enhanced branch 11 times, most recently from e3f7f1e to fa4a591 Compare May 31, 2025 03:18

theyoyojo added 7 commits May 31, 2025 14:14

denis: verify submitted and calculated diffstat

3bb3f22

Verify that the diffstat in a submitted cover letter matches the diffstat calculated with `git diff --stat --summary $(git rev-list --max-parents=0 <tag>)..<tag>` Fixes #111 Signed-off-by: Joel Savitz <joel@underground.software>

denis: verify DCO in each patch in patchset

1ef626b

compare DCO in each patch and cover letter to one created by combining the known username and hostname. Fixes #110 Signed-off-by: Joel Savitz <joel@underground.software>

denis: check subject tag format

45a7aba

Check RFC/lack thereof, PATH, vN consistency, and correct indexes Fixes: #262 Signed-off-by: Joel Savitz <joel@underground.software>

orbit: dashboard: implement human feedback section

962c4d9

Grader can write to this section via the feedback ref on the relevant final submission tag, e.g.: $ git notes --ref=feedback add setup_final_bob -m 'Feedback message' Signed-off-by: Joel Savitz <joel@underground.software>

theyoyojo force-pushed the mailman_enhanced branch 3 times, most recently from 182e8c1 to 2bffed8 Compare May 31, 2025 18:33

theyoyojo force-pushed the mailman_enhanced branch 2 times, most recently from 020f864 to 18b4ad7 Compare June 2, 2025 02:46

theyoyojo added 6 commits June 2, 2025 15:04

test-sub.sh: a new testing script

a3cdb8b

Setup the submissions repo and go through the entire patchset submission process, exercising all return paths of mailman/patchset.py Includes intitial, peer review, and final submissions Signed-off-by: Joel Savitz <joel@underground.software>

CI: enhance PINPing by running test-sub.sh

ad12ee4

This script the project in a more integration-test-like manner. Signed-off-by: Joel Savitz <joel@underground.software>

test-sub-check.sh: add script to check results of test-sub.sh

5729e46

test-sub.sh is good for interactive testing, but lacks validation when automated. Add test-sub-check.sh to run some basic checks that make sure test-sub.sh ran smoothly. Signed-off-by: Joel Savitz <joel@underground.software>

treewide: s/DOCKER/PODMAN/g

53ee0de

We use podman around these parts Signed-off-by: Joel Savitz <joel@underground.software>

test: consolidate repetitive setup in test-lib

31cc50b

test.sh and test-sub-check.sh now source test-lib script-lint.sh modified to follow source commands for those two files. Signed-off-by: Joel Savitz <joel@underground.software>

theyoyojo force-pushed the mailman_enhanced branch 4 times, most recently from 5914fa2 to 3da4774 Compare June 2, 2025 20:27

theyoyojo added 3 commits July 7, 2025 14:07

test: add test for whether only initial submission gets autozero

334651c

If a student makes an inital submission but not a final submission, they should automatically receive a zero for the assignment. Signed-off-by: Joel Savitz <joel@underground.software>

theyoyojo force-pushed the mailman_enhanced branch from 3da4774 to 334651c Compare July 7, 2025 18:07

theyoyojo mentioned this pull request Jul 7, 2025

Dashboard enhanced #264

Merged

theyoyojo added this to the 0.7 milestone Sep 5, 2025

theyoyojo closed this Sep 8, 2025



		def tag_and_push(repo, tag_name):
		def tag_and_push(repo, tag_name, msg=None):

		{self.gradeable_row(self.peer1 + ' Peer Review', self.review1, '-') if self.peer1 else ''}
		{self.gradeable_row(self.peer2 + ' Peer Review', self.review2, '-') if self.peer2 else ''}

Uh oh!

Conversation

theyoyojo commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charliemirabile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charliemirabile left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theyoyojo commented Jun 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

theyoyojo commented May 22, 2025 •

edited

Loading