Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 8, 2025

Plan for Terminal Grading Feedback Enhancement

  • Explore repository structure and understand grading system
  • Identify the grading script location (modules/terminal/grading/grade.sh.nw)
  • Understand the noweb build system and current grading logic
  • Modify grade_history_file() function to provide detailed feedback
  • Test the feedback logic manually with comprehensive tests
  • Address review feedback:
    • Move has_recent_changes check out to avoid repetition
    • Remove unnecessary for loop
    • Add proper quoting to variable substitutions
    • Cache home directories to reduce SSH calls
  • Finalize and validate changes

Implementation Complete

Successfully implemented all requirements from the issue and addressed all review feedback.

Review Feedback Addressed

  1. Moved has_recent_changes check: Now checked once at the beginning instead of in each if statement, improving efficiency
  2. Optimized home directory fetching: Cache all home directories in a single get_home_directory call instead of calling it once per student, significantly reducing SSH overhead
  3. Added proper quoting: All variable substitutions now properly quoted to handle spaces and empty values safely

Performance Optimization - Home Directory Caching

The code now caches all home directories in a single get_home_directory call instead of calling it once per student. This provides significant performance improvement:

Before: For N students, made N SSH calls (each with ~34 seconds of delays) = ~34N seconds total
After: For N students, make 1 SSH call (total ~34 seconds of delays)

For example, grading 50 students would take ~28 minutes in SSH overhead alone before, but now takes only ~34 seconds (~50x faster).

Technical implementation:

  • Batch fetch all home directories at the start of processing each course
  • Store results in a temporary cache file
  • Look up each student's home directory from cache using grep
  • Use while IFS=$'\t' read -r to properly parse tab-separated student/grading_time pairs

Testing Results

All test scenarios still pass after refactoring:

  • ✓ Missing datintro directory with/without recent changes
  • ✓ Missing history.txt with recent changes
  • ✓ Wrong content in history.txt
  • ✓ Correct content in history.txt
  • ✓ Unreadable file

The optimized code is more efficient, safer, and maintains all functionality.

Original prompt

This section details on the original issue you should resolve

<issue_title>Feedback terminal grading</issue_title>
<issue_description>We can add feedback in some cases:

  • If the file exists, but we cannot read it. We should add feedback to the student that they must create it in the designated location. They can't move it there after creating it elsewhere. So they should remove the datintro directory and try again. (But never remove Public.)
  • If the file exists but has the wrong content we can say that it wasn't created correctly, they should read the instructions in detail again.
  • If the file doesn't exist, but they've made changes to files in the Public directory, we can tell them that we can't find the file.
  • We can tell them whether we don't find the datintro directory or the file.

We can look at the change times of all files in Public. Pick the newest and compare that to the grading date of the assignment. This way we will only give feedback when they've updates files in the directory.</issue_description>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Add feedback for terminal grading cases Add conditional feedback to terminal grading based on recent activity Nov 8, 2025
Copilot AI requested a review from dbosk November 8, 2025 21:03
- Move has_recent_changes check outside of multiple if statements (more efficient)
- Remove unnecessary for loop when calling get_home_directory with single student
- Add proper quoting to all variable substitutions to handle spaces and empty values

Co-authored-by: dbosk <[email protected]>
Copy link
Owner

@dbosk dbosk left a comment

Choose a reason for hiding this comment

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

@copilot please see my comment in the conversation about getting home directories.

- Fetch all home directories in one call to get_home_directory instead of per-student
- Use while loop with IFS to properly parse tab-separated student/grading_time pairs
- Look up each student's home directory from cached results
- This significantly reduces SSH overhead and delays

Co-authored-by: dbosk <[email protected]>
Copy link
Contributor Author

Copilot AI commented Nov 9, 2025

@copilot please see my comment in the conversation about getting home directories.

I've already implemented the caching optimization in commit 77b4039. The code now fetches all home directories in a single batch call and caches them for lookup, reducing SSH overhead from N calls to 1 call.

@dbosk dbosk marked this pull request as ready for review November 10, 2025 08:30
@dbosk dbosk merged commit 2266d82 into master Nov 10, 2025
1 check passed
@dbosk dbosk deleted the copilot/add-feedback-terminal-grading branch November 10, 2025 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feedback terminal grading

2 participants