diff --git a/canvigator.py b/canvigator.py index af7814a..2529ce3 100755 --- a/canvigator.py +++ b/canvigator.py @@ -47,7 +47,7 @@ def print_help(): ch.print_global_help(task_groups) -def _run_assignment_task(task, course, canvas, canv_config, dry_run, auto_grade_flag): +def _run_assignment_task(task, course, canvas, canv_config, dry_run, auto_grade_flag, save_transcript=False): """Dispatch the media-recording-assignment tasks.""" import canvigator_assignment as ca if task == 'create-media-recording-assignment': @@ -59,7 +59,7 @@ def _run_assignment_task(task, course, canvas, canv_config, dry_run, auto_grade_ if task == 'analyze-media-recordings': cassign.analyzeRecordings() else: - cassign.getMediaRecordings(auto_grade=auto_grade_flag, dry_run=dry_run) + cassign.getMediaRecordings(auto_grade=auto_grade_flag, dry_run=dry_run, save_transcript=save_transcript) # Course-level tasks that need only `course` + a few primitives. Kept in a @@ -87,7 +87,7 @@ def _run_course_task(task, course, canv_config, n_days, cloud_questions): cd.prepClassDigest(course, days=n_days, cloud_questions=cloud_questions) -def _run_quiz_task(task, quiz, dry_run, tag, reply_window_days, send_all=False): +def _run_quiz_task(task, quiz, dry_run, tag, reply_window_days, send_all=False, choose_model=False, save_transcript=False): """Dispatch a quiz-level task to the appropriate method.""" if task == 'get-quiz-questions': quiz.getQuizQuestions(tag=tag) @@ -96,7 +96,14 @@ def _run_quiz_task(task, quiz, dry_run, tag, reply_window_days, send_all=False): quiz.getAllSubmissionsAndEvents() quiz.generateFirstAttemptHistograms() elif task == 'assess-replies': - quiz.assessFollowUpReplies(reply_window_days=reply_window_days) + chosen_model = None + if choose_model: + import canvigator_llm + chosen_model = canvigator_llm.pickLocalGemmaModel() + quiz.assessFollowUpReplies( + reply_window_days=reply_window_days, chosen_model=chosen_model, + save_transcript=save_transcript, + ) elif task == 'send-quiz-reminder': quiz.sendQuizReminders(dry_run=dry_run, send_all=send_all) elif task == 'send-follow-up-question': @@ -133,6 +140,8 @@ def _run_quiz_task(task, quiz, dry_run, tag, reply_window_days, send_all=False): '-n': '--days', '-q': '--cloud-questions', '-s': '--send-all', + '-M': '--choose-model', + '-S': '--save-transcript', '-h': '--help', } args = [_SHORT_TO_LONG.get(a, a) for a in sys.argv[1:]] @@ -173,6 +182,14 @@ def _run_quiz_task(task, quiz, dry_run, tag, reply_window_days, send_all=False): if cloud_questions_flag: args.remove('--cloud-questions') +choose_model_flag = '--choose-model' in args +if choose_model_flag: + args.remove('--choose-model') + +save_transcript_flag = '--save-transcript' in args +if save_transcript_flag: + args.remove('--save-transcript') + crn = None if '--crn' in args: crn_idx = args.index('--crn') @@ -358,7 +375,7 @@ def _run_quiz_task(task, quiz, dry_run, tag, reply_window_days, send_all=False): course.sendAllQuizReminders(dry_run=dry_run, send_all=send_all) elif task in ('create-media-recording-assignment', 'get-media-recordings', 'analyze-media-recordings'): - _run_assignment_task(task, course, canvas, canv_config, dry_run, auto_grade_flag) + _run_assignment_task(task, course, canvas, canv_config, dry_run, auto_grade_flag, save_transcript=save_transcript_flag) elif task == 'generate-follow-up-questions': # Driven by a pre-selected tagged-questions CSV, not a Canvas quiz @@ -392,6 +409,9 @@ def _run_quiz_task(task, quiz, dry_run, tag, reply_window_days, send_all=False): print(f"\nSelected quiz: {quiz_choice.title}") skip = task in ('get-quiz-questions', 'assess-replies', 'send-follow-up-assessments') quiz = cq.CanvigatorQuiz(canvas, course, quiz_choice, canv_config, verbose=False, skip_student_data=skip) - _run_quiz_task(task, quiz, dry_run, tag, reply_window_days, send_all=send_all) + _run_quiz_task( + task, quiz, dry_run, tag, reply_window_days, + send_all=send_all, choose_model=choose_model_flag, save_transcript=save_transcript_flag, + ) print("\n*** Done ***\n") diff --git a/canvigator_assignment.py b/canvigator_assignment.py index 981b037..2980f86 100644 --- a/canvigator_assignment.py +++ b/canvigator_assignment.py @@ -355,7 +355,7 @@ def _writeRecordingsCsv(self, rows): df.to_csv(csv_path, index=False) return csv_path, df - def getMediaRecordings(self, auto_grade=False, dry_run=False): + def getMediaRecordings(self, auto_grade=False, dry_run=False, save_transcript=False): """Fetch every submission, transcribe locally, optionally review and grade interactively. For each submitter, ffmpeg pulls the media URL directly (handling DASH @@ -410,7 +410,7 @@ def getMediaRecordings(self, auto_grade=False, dry_run=False): rows.append(self._buildRecordingRow(sub, student_name, audio_rel, "")) continue print(" transcribing...", end="", flush=True) - transcript = transcribe_audio(str(local_path), client, audio_model) + transcript = transcribe_audio(str(local_path), client, audio_model, save_transcript=save_transcript) chars = len(transcript) print(f" done ({chars} chars)") diff --git a/canvigator_course.py b/canvigator_course.py index 022ec0e..ad0ac72 100644 --- a/canvigator_course.py +++ b/canvigator_course.py @@ -1154,6 +1154,12 @@ def exportAnonymizedData(data_path): n_total = len(csv_files) print(f"Exported {n_total} file{'s' if n_total != 1 else ''} ({n_anonymized} anonymized) to {anon_dir.name}/") logger.info(f"Exported {n_total} anonymized files to {zip_path}.zip") + print( + "\nNOTE: This export is NOT automatically privacy-safe. Before sharing, " + "manually verify the CSVs contain no student IDs or names, and no " + "open-ended text (transcripts, replies, feedback, conversation subjects), " + "audio, or images." + ) def _renderQuestionPreview(question_dict): diff --git a/canvigator_help.py b/canvigator_help.py index 077d58f..6213938 100644 --- a/canvigator_help.py +++ b/canvigator_help.py @@ -68,6 +68,18 @@ "Requires a matching --dry-run run on the same task within the last " "10 minutes (otherwise refuses). Mutually exclusive with --dry-run.", ), + '--choose-model': ( + '-M', None, + "Interactively pick which local gemma4 model to use for grading; " + "defaults to OLLAMA_MODEL. Useful for trading some assessment quality " + "for throughput (e.g. swap gemma4:31b for gemma4:e4b).", + ), + '--save-transcript': ( + '-S', None, + "Save each audio transcription as a sibling .txt file alongside the " + "audio (debug aid for noisy or mistranscribed recordings — listen + " + "read side by side).", + ), } @@ -571,10 +583,12 @@ "data//quiz_followup_assessments.csv (persistent; " "instructor edits 'feedback' before send-follow-up-assessments).", ], - 'flags': ['--reply-window-days'], + 'flags': ['--reply-window-days', '--choose-model', '--save-transcript'], 'examples': [ "python canvigator.py --crn 12345 assess-replies", "python canvigator.py --crn 12345 -w 10 assess-replies", + "python canvigator.py --crn 12345 -M assess-replies", + "python canvigator.py --crn 12345 -S assess-replies", ], 'run_before': ['send-follow-up-question'], 'run_after': ['send-follow-up-assessments', 'prep-class-digest'], @@ -675,11 +689,12 @@ "audio_path, transcript, transcribed_at, grade, graded_at).", "Canvas: posted_grade on each submission (skipped under --dry-run).", ], - 'flags': ['--auto-grade', '--dry-run'], + 'flags': ['--auto-grade', '--dry-run', '--save-transcript'], 'examples': [ "python canvigator.py --crn 12345 get-media-recordings", "python canvigator.py --crn 12345 --auto-grade --dry-run " "get-media-recordings", + "python canvigator.py --crn 12345 -S get-media-recordings", ], 'run_before': ['create-media-recording-assignment'], 'run_after': ['analyze-media-recordings', 'prep-class-digest'], diff --git a/canvigator_llm.py b/canvigator_llm.py index 4867175..604fc96 100644 --- a/canvigator_llm.py +++ b/canvigator_llm.py @@ -2,12 +2,36 @@ import html import json import logging +import math import os import re +import subprocess +import tempfile import time +from pathlib import Path + +from canvigator_utils import selectFromList logger = logging.getLogger(__name__) + +def _isValidMediaFile(path, min_bytes=1024): + """Return True iff ``path`` exists on disk and is at least ``min_bytes`` bytes. + + Preflight before ``transcribe_audio`` / ``assess_draw`` — a missing or + near-empty file means the student's Canvas submission didn't carry usable + media, and feeding it to Gemma wastes ~5 min per self-consistency vote. + The 1 KB default filters out empty WAV headers (~44 B) and zero-byte + image placeholders while permitting very short legitimate recordings. + """ + if not path: + return False + try: + return os.path.isfile(path) and os.path.getsize(path) >= min_bytes + except OSError: + return False + + # Retry policy for transient upstream errors (e.g. Ollama cloud 5xx). _CHAT_MAX_ATTEMPTS = 4 _CHAT_BACKOFF_BASE_SECS = 1.0 @@ -66,6 +90,28 @@ def _make_client(cloud=False): return ollama.Client() +def pickLocalGemmaModel(): + """Prompt the instructor to choose from local ``gemma4:*`` models. + + Returns the chosen model name (e.g. ``'gemma4:31b'``). Raises ``RuntimeError`` + when no gemma4 models are installed locally — the assess-replies grading + path requires one. Uses the existing local Ollama client (no API key). + """ + client = _make_client(cloud=False) + resp = client.list() + models = getattr(resp, 'models', None) or [] + gemma_names = sorted( + m.model for m in models + if isinstance(getattr(m, 'model', None), str) and m.model.startswith('gemma4:') + ) + if not gemma_names: + raise RuntimeError( + "No local gemma4 models found. Run 'ollama pull gemma4:31b' " + "(or another gemma4 model) and try again." + ) + return selectFromList(gemma_names, item_type="local gemma4 model") + + _TAG_SYSTEM_PROMPT = ( "You are a concise topic tagger for university quiz questions. " "Given a question, respond with 1 to 3 short concept tags that describe " @@ -1054,17 +1100,52 @@ def _build_assessment_prompt( return "\n\n".join(parts) -def transcribe_audio(audio_path, client, model): - """Transcribe an audio file using a multimodal model (e.g. gemma4:e4b).""" +# Gemma 4's audio encoder has a ~30-second window — audio longer than this +# is silently truncated by the model, which causes the transcript to loop on +# whatever was said in the first 30s (observed: 79s WAV → transcript was 5x +# repetition of the student's opening sentence; the actual answer in 30-79s +# was never seen). We chunk longer audio with ffmpeg so every second is +# given to the model in its own call. +_AUDIO_CHUNK_SECS = 30 + + +def _probe_audio_duration_secs(audio_path): + """Return audio duration in seconds via ffprobe, or 0.0 on failure. + + A 0.0 result causes ``transcribe_audio`` to fall through to single-pass + behavior — chunking is best-effort and never blocks transcription. + """ + try: + result = subprocess.run( + ['ffprobe', '-v', 'error', '-show_entries', 'format=duration', + '-of', 'default=noprint_wrappers=1:nokey=1', str(audio_path)], + capture_output=True, text=True, check=True, timeout=10, + ) + return float(result.stdout.strip()) + except (subprocess.CalledProcessError, ValueError, FileNotFoundError, + subprocess.TimeoutExpired) as e: + logger.warning(f"ffprobe duration failed for {audio_path}: {e}") + return 0.0 + + +def _transcribe_one_pass(audio_path, client, model): + """Single-pass transcription of an audio file. Returns text or empty string on error. + + ``repeat_penalty`` discourages the runaway repetition loops Gemma 4 falls + into when the speaker stutters at the start of a clip (without it, the + first chunk of a 79s WAV generated 640KB of looped output over 40 minutes). + ``num_predict`` caps any residual loop at ~5 minutes of speech worth of + tokens — a hard safety net. + """ try: resp = _chat_with_retry( client, model=model, messages=[ {"role": "system", "content": _TRANSCRIBE_SYSTEM_PROMPT}, - {"role": "user", "content": "Transcribe this audio recording.", "images": [audio_path]}, + {"role": "user", "content": "Transcribe this audio recording.", "images": [str(audio_path)]}, ], - options={"temperature": 0.1}, + options={"temperature": 0.1, "repeat_penalty": 1.3, "num_predict": 512}, ) return resp["message"]["content"].strip() except Exception as e: @@ -1072,6 +1153,59 @@ def transcribe_audio(audio_path, client, model): return "" +def transcribe_audio(audio_path, client, model, save_transcript=False): + """Transcribe an audio file using a multimodal model (e.g. gemma4:e4b). + + Audio longer than ``_AUDIO_CHUNK_SECS`` is split into segments with ffmpeg + and each segment is transcribed in its own call, because Gemma 4's audio + encoder window is ~30s and silently drops the remainder otherwise. The + per-segment transcripts are joined with spaces. When ffprobe is unavailable + or the probe fails, behavior falls through to a single pass (matches the + prior implementation). + + When ``save_transcript`` is True, also writes the (possibly concatenated) + transcript to a sibling ``.txt`` file next to ``audio_path`` (e.g. + ``foo.wav`` → ``foo.txt``) as a debug aid. An empty transcript still + produces a zero-byte ``.txt`` — file present means transcription was + attempted; size 0 means the model returned nothing. Write failures are + logged and never block the return. + """ + audio_path = Path(audio_path) + duration = _probe_audio_duration_secs(audio_path) + if duration > _AUDIO_CHUNK_SECS: + n_chunks = math.ceil(duration / _AUDIO_CHUNK_SECS) + segments = [] + with tempfile.TemporaryDirectory(prefix='canv_chunks_') as tmpdir: + tmpdir = Path(tmpdir) + for i in range(n_chunks): + chunk_path = tmpdir / f"chunk_{i:03d}.wav" + try: + subprocess.run( + ['ffmpeg', '-y', '-loglevel', 'error', + '-ss', str(i * _AUDIO_CHUNK_SECS), + '-i', str(audio_path), + '-t', str(_AUDIO_CHUNK_SECS), + '-ar', '16000', '-ac', '1', '-c:a', 'pcm_s16le', + str(chunk_path)], + check=True, capture_output=True, timeout=60, + ) + except (subprocess.CalledProcessError, FileNotFoundError, + subprocess.TimeoutExpired) as e: + logger.warning(f"ffmpeg chunk {i} failed for {audio_path}: {e}") + continue + segments.append(_transcribe_one_pass(chunk_path, client, model)) + transcript = ' '.join(s for s in segments if s) + else: + transcript = _transcribe_one_pass(audio_path, client, model) + if save_transcript: + txt_path = audio_path.with_suffix('.txt') + try: + txt_path.write_text(transcript) + except OSError as e: + logger.warning(f"Failed to write transcript to {txt_path}: {e}") + return transcript + + def _assess_explain_once(prompt, client, model): """Single-pass explain assessment. Returns (result, feedback, evaluations_dict).""" try: @@ -1186,7 +1320,8 @@ def _parse_rubric_from_row(question_info_row): def assess_replies(replies, question_info_row, model=None, audio_model=None, - locked_examples=None, n_consistency=_SELF_CONSISTENCY_N): + locked_examples=None, n_consistency=_SELF_CONSISTENCY_N, + save_transcript=False): """Assess a list of student reply dicts, returning a list of assessment result dicts. Each reply dict should have keys: student_id, student_name, question_id, @@ -1229,35 +1364,48 @@ def assess_replies(replies, question_info_row, model=None, audio_model=None, total = len(replies) print(f"Assessing {total} student replies with model '{model}' (n={n_consistency} self-consistency)...") results = [] + run_start = time.monotonic() for i, reply in enumerate(replies, start=1): + student_start = time.monotonic() student_name = reply.get('student_name', '?') mode = reply.get('question_mode', 'explain') print(f" [{i}/{total}] {student_name} ({mode})...", end="", flush=True) transcript = '' evaluations = {} + + def _skip_row(feedback_msg): + """Build a uniform skip-row matching the graded-row shape.""" + return { + 'student_id': reply['student_id'], + 'student_name': student_name, + 'question_id': reply['question_id'], + 'question_mode': mode, + 'result': 'fail', + 'confidence': 'high', + 'feedback': feedback_msg, + 'transcript': '', + 'criteria_evaluations': '', + 'assessed_at': '', + } + if mode == 'explain': audio_path = reply.get('audio_path', '') - if audio_path: - print(" transcribing...", end="", flush=True) - transcript = transcribe_audio(audio_path, client, audio_model) - if not transcript: - # Fall back to reply text if no audio or transcription failed - transcript = _strip_html(reply.get('reply_text', '')) + # Strict preflight: a missing or near-empty audio file means the + # student's Canvas submission didn't carry a usable recording. + # Skip without calling Gemma — feeding nothing to the audio model + # was costing ~5 min/student before this check. + if not _isValidMediaFile(audio_path): + elapsed_min = (time.monotonic() - student_start) / 60 + print(f" no valid audio — skipped... processed in {elapsed_min:.1f}min") + results.append(_skip_row('No valid audio submission to assess.')) + continue + print(" transcribing...", end="", flush=True) + transcript = transcribe_audio(audio_path, client, audio_model, save_transcript=save_transcript) if not transcript: - print(" no response content — skipped") - results.append({ - 'student_id': reply['student_id'], - 'student_name': student_name, - 'question_id': reply['question_id'], - 'question_mode': mode, - 'result': 'fail', - 'confidence': 'high', - 'feedback': 'No response content to assess (no audio and no text).', - 'transcript': '', - 'criteria_evaluations': '', - 'assessed_at': '', - }) + elapsed_min = (time.monotonic() - student_start) / 60 + print(f" transcription returned empty — skipped... processed in {elapsed_min:.1f}min") + results.append(_skip_row('Audio transcription returned empty.')) continue print(f" assessing (x{n_consistency})...", end="", flush=True) result, confidence, feedback, evaluations = assess_explain( @@ -1268,20 +1416,10 @@ def assess_replies(replies, question_info_row, model=None, audio_model=None, else: # Draw mode image_path = reply.get('attachment_path', '') - if not image_path: - print(" no image attached — skipped") - results.append({ - 'student_id': reply['student_id'], - 'student_name': student_name, - 'question_id': reply['question_id'], - 'question_mode': mode, - 'result': 'fail', - 'confidence': 'high', - 'feedback': 'No image attachment to assess.', - 'transcript': '', - 'criteria_evaluations': '', - 'assessed_at': '', - }) + if not _isValidMediaFile(image_path): + elapsed_min = (time.monotonic() - student_start) / 60 + print(f" no valid image — skipped... processed in {elapsed_min:.1f}min") + results.append(_skip_row('No valid image submission to assess.')) continue print(f" assessing image (x{n_consistency})...", end="", flush=True) # locked_examples carry transcripts, which don't apply to drawings — drop them. @@ -1293,7 +1431,8 @@ def assess_replies(replies, question_info_row, model=None, audio_model=None, from datetime import datetime, timezone assessed_at = datetime.now(timezone.utc).isoformat() - print(f" {result} ({confidence})") + elapsed_min = (time.monotonic() - student_start) / 60 + print(f" {result} ({confidence})... processed in {elapsed_min:.1f}min") results.append({ 'student_id': reply['student_id'], @@ -1309,6 +1448,8 @@ def assess_replies(replies, question_info_row, model=None, audio_model=None, }) print("Assessment complete.") + total_min = (time.monotonic() - run_start) / 60 + print(f"Total time: {total_min:.1f}min") return results diff --git a/canvigator_quiz.py b/canvigator_quiz.py index e6e6b4a..8fbc3dc 100644 --- a/canvigator_quiz.py +++ b/canvigator_quiz.py @@ -16,6 +16,7 @@ from datetime import datetime, timedelta, timezone from pathlib import Path from canvigator_utils import today_str, selectCSVFromList, find_latest_csv, spin, spin_done, format_due_date +from canvigator_assignment import _fetchAudio logger = logging.getLogger(__name__) @@ -42,6 +43,108 @@ def _parse_points_possible_from_student_analysis(csv_path): return points_possible +def _firstNameForFilename(student_name): + """Return the student's first name as a filename-safe token. + + Used to inject readable identifiers into reply filenames so the + instructor can match a recording or drawing to a student at a glance + without consulting the CSV. Falls back to ``'student'`` when the name + is missing or whitespace-only; path separators are replaced with + underscores defensively (Canvas names won't normally contain them). + """ + if not isinstance(student_name, str): + student_name = '' if student_name is None else str(student_name) + tokens = student_name.split() + if not tokens or tokens[0].lower() == 'nan': + return 'student' + return tokens[0].replace('/', '_').replace('\\', '_') + + +_RESIZABLE_IMAGE_EXTS = {'.jpg', '.jpeg', '.png', '.webp', '.gif', '.bmp', '.tiff'} +_MAX_IMAGE_EDGE = 1024 # px; long-edge cap for draw-mode attachments + + +def _resizeImageInPlace(image_path, max_edge=_MAX_IMAGE_EDGE, jpeg_quality=85): + """Cap an image's long edge to ``max_edge`` pixels, overwriting in place. + + Phone-camera photos can be 3000+ px on the long edge and several MB on + disk. Every draw-mode self-consistency pass re-encodes the full image + and feeds those pixels through gemma4:31b's vision encoder, whose + latency scales roughly linearly with input size. Down-sampling to + 1024 px keeps hand-drawn diagrams legible while substantially cutting + per-call inference time. Aspect ratio and format are preserved; a + no-op when the image is already small enough. Returns True on success + (including the no-op path); False when Pillow is missing or the file + can't be opened (caller logs + continues). + """ + try: + from PIL import Image + except ImportError: + logger.warning(f"Pillow not installed; skipping resize of {image_path.name}.") + return False + try: + with Image.open(str(image_path)) as img: + img.load() + fmt = img.format + if max(img.size) <= max_edge: + return True + img.thumbnail((max_edge, max_edge), Image.Resampling.LANCZOS) + save_kwargs = {} + if fmt == 'JPEG': + save_kwargs = {'quality': jpeg_quality, 'optimize': True} + elif fmt == 'PNG': + save_kwargs = {'optimize': True} + img.save(str(image_path), format=fmt, **save_kwargs) + return True + except Exception as e: + logger.warning(f"Failed to resize {image_path.name}: {e}") + return False + + +def _rasterizePdfToPng(pdf_path, png_path, dpi=100): + """Rasterize page 1 of a PDF to a PNG via PyMuPDF. + + Returns True on success, False on failure (caller logs + skips). Some + students submit phone-scanner output (Adobe Scan, CamScanner) as PDFs; + Ollama's Gemma multimodal only accepts PNG/JPEG/WebP/GIF, so the raw + PDF triggers ``image: unknown format``. Logs a warning if the PDF has + multiple pages — a single follow-up answer should fit on one page; + extra pages are dropped. The default DPI is 100 (≈850×1100 px for a + US Letter page), enough fidelity for hand-drawn diagrams without + creating a multi-MB intermediate that ``_resizeImageInPlace`` would + immediately shrink. + """ + try: + import pymupdf + except ImportError: + logger.warning( + f"pymupdf not installed; cannot convert {pdf_path.name} to image. " + "Install with 'pip install pymupdf'." + ) + return False + try: + doc = pymupdf.open(str(pdf_path)) + except Exception as e: + logger.warning(f"Failed to open {pdf_path.name} as PDF: {e}") + return False + try: + if doc.page_count == 0: + logger.warning(f"{pdf_path.name}: PDF has no pages") + return False + if doc.page_count > 1: + logger.warning( + f"{pdf_path.name}: PDF has {doc.page_count} pages; rasterizing page 1 only" + ) + pix = doc.load_page(0).get_pixmap(dpi=dpi) + pix.save(str(png_path)) + return True + except Exception as e: + logger.warning(f"Failed to rasterize {pdf_path.name}: {e}") + return False + finally: + doc.close() + + def _hasRecentDryRunManifest(data_path, pattern, max_age_minutes=10): """Return True iff a ``*_dryrun_*.csv`` mtime is within ``max_age_minutes``. @@ -1241,7 +1344,7 @@ def getFollowUpReplies(self, reply_window_days=5): for i, msg in enumerate(student_replies): is_latest = (i == 0) # messages are newest-first from Canvas attachment_path, audio_path = self._downloadReplyMedia( - msg, student_id, question_id, file_prefix, replies_dir + msg, student_id, student_name, question_id, file_prefix, replies_dir ) replied_at = msg.get('created_at', '') @@ -1333,23 +1436,28 @@ def _extractStudentReplies(self, messages, instructor_id, sent_at, cutoff): replies.append(msg) return replies - def _downloadReplyMedia(self, msg, student_id, question_id, file_prefix, replies_dir): + def _downloadReplyMedia(self, msg, student_id, student_name, question_id, file_prefix, replies_dir): """Download any attachment or audio from a reply message. Returns (attachment_path, audio_path) — each is a string path relative - to the data directory, or None if no media of that type. + to the data directory, or None if no media of that type. The student's + first name is embedded in the filename (``...__.``) + so the instructor can recognize files at a glance. """ attachment_path = None audio_path = None + first_name = _firstNameForFilename(student_name) - # Download image/file attachments + # Download image/file attachments. PDFs (phone-scanner output) get + # rasterized to PNG so draw-mode assessment can hand a real image to + # Ollama; the original .pdf is left on disk for instructor reference. attachments = msg.get('attachments', []) if attachments: att = attachments[0] # Take the first attachment att_url = att.get('url', '') filename = att.get('filename', '') or att.get('display_name', 'attachment') ext = os.path.splitext(filename)[1] or '.bin' - local_name = f"{file_prefix}{student_id}_{question_id}{ext}" + local_name = f"{file_prefix}{student_id}_{first_name}_{question_id}{ext}" local_path = replies_dir / local_name try: resp = requests.get(att_url, timeout=30) @@ -1358,30 +1466,38 @@ def _downloadReplyMedia(self, msg, student_id, question_id, file_prefix, replies f.write(resp.content) attachment_path = str(local_path.relative_to(self.config.data_path.parent)) logger.info(f"Downloaded attachment for student {student_id}: {local_name}") + if ext.lower() == '.pdf': + png_path = local_path.with_suffix('.png') + if _rasterizePdfToPng(local_path, png_path): + _resizeImageInPlace(png_path) + attachment_path = str(png_path.relative_to(self.config.data_path.parent)) + logger.info(f"Rasterized PDF for student {student_id}: {png_path.name}") + elif ext.lower() in _RESIZABLE_IMAGE_EXTS: + _resizeImageInPlace(local_path) except Exception as e: logger.warning(f"Failed to download attachment for student {student_id}: {e}") - # Download audio/video media comment + # Download audio/video media comment. Canvas serves media-recording + # playback as a DASH manifest, not a raw audio file, so a plain GET + # writes the manifest XML to disk (unplayable). ffmpeg follows the + # manifest, pulls the fragments, and re-encodes to 16 kHz mono PCM + # WAV — the only format Ollama's media detection routes to the + # audio path on Gemma multimodal (AAC-in-m4a is misclassified as + # `image: unknown format`). media_comment = msg.get('media_comment') if media_comment: media_url = media_comment.get('url', '') - media_type = media_comment.get('media_type', 'audio') - ext = '.m4a' if media_type == 'audio' else '.mp4' - local_name = f"{file_prefix}{student_id}_{question_id}{ext}" + local_name = f"{file_prefix}{student_id}_{first_name}_{question_id}.wav" local_path = replies_dir / local_name - try: - resp = requests.get(media_url, timeout=60) - resp.raise_for_status() - with open(local_path, 'wb') as f: - f.write(resp.content) + if _fetchAudio(media_url, local_path): audio_path = str(local_path.relative_to(self.config.data_path.parent)) logger.info(f"Downloaded media for student {student_id}: {local_name}") - except Exception as e: - logger.warning(f"Failed to download media for student {student_id}: {e}") + else: + logger.warning(f"Failed to download media for student {student_id}: {local_name}") return attachment_path, audio_path - def assessFollowUpReplies(self, reply_window_days=5): + def assessFollowUpReplies(self, reply_window_days=5, chosen_model=None, save_transcript=False): """Fetch the latest student replies, then assess them with the local LLM. First refreshes replies from Canvas (writing the @@ -1390,7 +1506,11 @@ def assessFollowUpReplies(self, reply_window_days=5): to produce a pass/fail assessment with feedback. Merges results into a single persistent followup_assessments CSV (no date suffix); rows where ``sent_assessment=1`` are preserved verbatim so previously sent feedback - is never overwritten by re-runs. + is never overwritten by re-runs. ``chosen_model`` overrides the default + grading model (``OLLAMA_MODEL``) when set — used by ``--choose-model`` + to let the instructor pick a smaller local gemma4 at runtime. + ``save_transcript`` writes each audio transcription to a sibling .txt + for debugging (``--save-transcript``). """ self.getFollowUpReplies(reply_window_days=reply_window_days) replies_df = self._loadFollowUpReplies() @@ -1449,7 +1569,10 @@ def assessFollowUpReplies(self, reply_window_days=5): print(f" Using {len(locked_examples)} instructor-approved example(s) for calibration.") import canvigator_llm - results = canvigator_llm.assess_replies(to_assess, oe_row, locked_examples=locked_examples) + results = canvigator_llm.assess_replies( + to_assess, oe_row, model=chosen_model, + locked_examples=locked_examples, save_transcript=save_transcript, + ) # Carry conversation_id from the reply (or fall back to manifest lookup). convo_lookup = self._buildConversationLookup() diff --git a/requirements.txt b/requirements.txt index 43e6871..82bf75f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -6,3 +6,4 @@ requests==2.33.0 scipy==1.13.1 seaborn==0.13.2 ollama==0.4.7 +pymupdf==1.27.2.3 diff --git a/test_canvigator.py b/test_canvigator.py index ad919c2..261f0f7 100644 --- a/test_canvigator.py +++ b/test_canvigator.py @@ -3902,3 +3902,318 @@ def test_short_forms_also_rejected(self): rc, stdout, _ = self._run_cli('-d', '-s', 'send-follow-up-assessments') assert rc != 0 assert 'mutually exclusive' in stdout + + +class TestFirstNameForFilename: + """`_firstNameForFilename` produces a filename-safe first-name token.""" + + def test_takes_first_token_of_multi_word_name(self): + """A full name is split on whitespace; only the first token is returned.""" + from canvigator_quiz import _firstNameForFilename + assert _firstNameForFilename("Jane Doe Smith") == "Jane" + + def test_single_word_name_passes_through(self): + """A single-token name is returned as-is (no trailing chars).""" + from canvigator_quiz import _firstNameForFilename + assert _firstNameForFilename("Madonna") == "Madonna" + + def test_empty_or_whitespace_falls_back_to_student(self): + """Missing/empty/whitespace-only names fall back to 'student' so filenames stay valid.""" + from canvigator_quiz import _firstNameForFilename + for raw in ("", " ", None, float('nan')): + assert _firstNameForFilename(raw) == "student" + + def test_path_separators_are_sanitized(self): + """Any forward/back slashes are replaced so the token never escapes the replies dir.""" + from canvigator_quiz import _firstNameForFilename + assert _firstNameForFilename("Jane/Doe Smith") == "Jane_Doe" + assert _firstNameForFilename(r"Jane\Doe Smith") == "Jane_Doe" + + +class TestResizeImageInPlace: + """`_resizeImageInPlace` caps the long edge so draw-mode passes don't feed multi-MB images to gemma.""" + + @staticmethod + def _make_image(path, size, fmt='PNG', color='red'): + """Write a solid-color image of ``size`` at ``path`` in ``fmt``.""" + from PIL import Image + Image.new('RGB', size, color=color).save(str(path), format=fmt) + + def test_shrinks_image_larger_than_max_edge(self, tmp_path): + """A 3000×2000 image is shrunk so the long edge is at most 1024 px; aspect ratio is preserved.""" + from PIL import Image + from canvigator_quiz import _resizeImageInPlace + img_path = tmp_path / "big.jpg" + self._make_image(img_path, (3000, 2000), fmt='JPEG') + assert _resizeImageInPlace(img_path, max_edge=1024) is True + with Image.open(str(img_path)) as out: + assert max(out.size) == 1024 + # 3000/2000 = 1.5 → expect ~1024×683 + assert abs(out.size[0] / out.size[1] - 1.5) < 0.01 + + def test_leaves_small_image_untouched(self, tmp_path): + """An image already under max_edge is not re-saved (byte-for-byte identical).""" + from canvigator_quiz import _resizeImageInPlace + img_path = tmp_path / "small.png" + self._make_image(img_path, (200, 150), fmt='PNG') + original_bytes = img_path.read_bytes() + assert _resizeImageInPlace(img_path, max_edge=1024) is True + assert img_path.read_bytes() == original_bytes + + def test_preserves_format(self, tmp_path): + """The on-disk format after resize matches the original (PNG stays PNG, JPEG stays JPEG).""" + from PIL import Image + from canvigator_quiz import _resizeImageInPlace + for fmt, ext in (('PNG', '.png'), ('JPEG', '.jpg')): + img_path = tmp_path / f"big{ext}" + self._make_image(img_path, (2000, 2000), fmt=fmt) + assert _resizeImageInPlace(img_path, max_edge=512) is True + with Image.open(str(img_path)) as out: + assert out.format == fmt + + def test_returns_false_when_file_is_not_an_image(self, tmp_path, caplog): + """A non-image file logs a warning and returns False without crashing.""" + import logging as _logging + from canvigator_quiz import _resizeImageInPlace + bogus = tmp_path / "not_an_image.png" + bogus.write_bytes(b"not an image") + with caplog.at_level(_logging.WARNING, logger='canvigator_quiz'): + assert _resizeImageInPlace(bogus) is False + assert any('Failed to resize' in r.message for r in caplog.records) + + +class TestRasterizePdfToPng: + """`_rasterizePdfToPng` converts student PDF submissions to PNG for draw-mode assessment.""" + + @staticmethod + def _make_pdf(path, n_pages=1): + """Write a minimal n-page PDF at ``path`` using pymupdf.""" + import pymupdf + doc = pymupdf.open() + for _ in range(n_pages): + doc.new_page() + doc.save(str(path)) + doc.close() + + def test_rasterizes_single_page_pdf_to_png(self, tmp_path): + """A 1-page PDF is converted to a valid PNG at the requested path.""" + from canvigator_quiz import _rasterizePdfToPng + pdf = tmp_path / "submission.pdf" + png = tmp_path / "submission.png" + self._make_pdf(pdf, n_pages=1) + assert _rasterizePdfToPng(pdf, png) is True + assert png.exists() and png.stat().st_size > 0 + # PNG files start with the 8-byte signature \x89PNG\r\n\x1a\n. + assert png.read_bytes()[:8] == b'\x89PNG\r\n\x1a\n' + + def test_multi_page_pdf_warns_and_rasterizes_first_page(self, tmp_path, caplog): + """Multi-page PDFs log a warning but still produce a page-1 PNG.""" + import logging as _logging + from canvigator_quiz import _rasterizePdfToPng + pdf = tmp_path / "multi.pdf" + png = tmp_path / "multi.png" + self._make_pdf(pdf, n_pages=3) + with caplog.at_level(_logging.WARNING, logger='canvigator_quiz'): + result = _rasterizePdfToPng(pdf, png) + assert result is True + assert png.exists() + assert any('3 pages' in r.message for r in caplog.records) + + def test_returns_false_when_file_is_not_pdf(self, tmp_path, caplog): + """A non-PDF file returns False and logs a warning instead of crashing.""" + import logging as _logging + from canvigator_quiz import _rasterizePdfToPng + not_pdf = tmp_path / "not_a_pdf.pdf" + not_pdf.write_bytes(b"this is not a pdf") + png = tmp_path / "out.png" + with caplog.at_level(_logging.WARNING, logger='canvigator_quiz'): + result = _rasterizePdfToPng(not_pdf, png) + assert result is False + assert not png.exists() + + +class TestIsValidMediaFile: + """`_isValidMediaFile` is the preflight that skips Gemma when the student's media is missing.""" + + def test_empty_path_returns_false(self): + """An empty string path means the CSV column was unset for this reply.""" + from canvigator_llm import _isValidMediaFile + assert _isValidMediaFile('') is False + assert _isValidMediaFile(None) is False + + def test_nonexistent_path_returns_false(self, tmp_path): + """A path pointing at no file on disk fails preflight (no Gemma call).""" + from canvigator_llm import _isValidMediaFile + assert _isValidMediaFile(str(tmp_path / "does_not_exist.wav")) is False + + def test_zero_byte_file_returns_false(self, tmp_path): + """An empty file (e.g. ffmpeg produced nothing) fails preflight.""" + from canvigator_llm import _isValidMediaFile + empty = tmp_path / "empty.wav" + empty.touch() + assert _isValidMediaFile(str(empty)) is False + + def test_file_below_threshold_returns_false(self, tmp_path): + """A 500-byte file (below the 1 KB default) is treated as a header-only stub.""" + from canvigator_llm import _isValidMediaFile + tiny = tmp_path / "tiny.wav" + tiny.write_bytes(b'\x00' * 500) + assert _isValidMediaFile(str(tiny)) is False + + def test_file_above_threshold_returns_true(self, tmp_path): + """A 2 KB file passes the default threshold — enough to carry meaningful audio/image.""" + from canvigator_llm import _isValidMediaFile + ok = tmp_path / "ok.wav" + ok.write_bytes(b'\x00' * 2048) + assert _isValidMediaFile(str(ok)) is True + + +class TestPickLocalGemmaModel: + """`pickLocalGemmaModel` filters local Ollama models to gemma4-only and prompts the instructor.""" + + @staticmethod + def _mock_list_response(model_names): + """Build a SimpleNamespace mirroring the real ollama ListResponse shape.""" + return SimpleNamespace(models=[SimpleNamespace(model=n) for n in model_names]) + + def test_filters_to_gemma4_only(self, monkeypatch): + """When the local ollama has mixed families, only gemma4:* names reach the picker.""" + import canvigator_llm + mock_client = MagicMock() + mock_client.list.return_value = self._mock_list_response([ + 'gemma4:31b', 'qwen3:4b', 'gemma4:e4b', 'llama3.2:1b', + ]) + monkeypatch.setattr(canvigator_llm, '_make_client', lambda cloud=False: mock_client) + captured = {} + + def fake_select(items, item_type="item"): + captured['items'] = list(items) + return items[0] + + monkeypatch.setattr(canvigator_llm, 'selectFromList', fake_select) + result = canvigator_llm.pickLocalGemmaModel() + # Only gemma4 names should have been offered, sorted alphabetically. + assert captured['items'] == ['gemma4:31b', 'gemma4:e4b'] + assert result == 'gemma4:31b' + + def test_raises_when_no_gemma4_installed(self, monkeypatch): + """Empty gemma4 list raises a RuntimeError with a fix-it message.""" + import canvigator_llm + mock_client = MagicMock() + mock_client.list.return_value = self._mock_list_response(['qwen3:4b', 'llama3.2:1b']) + monkeypatch.setattr(canvigator_llm, '_make_client', lambda cloud=False: mock_client) + with pytest.raises(RuntimeError, match='No local gemma4'): + canvigator_llm.pickLocalGemmaModel() + + def test_returns_single_gemma_through_picker(self, monkeypatch): + """Even with a single gemma4 model the picker is still invoked (consistent UX).""" + import canvigator_llm + mock_client = MagicMock() + mock_client.list.return_value = self._mock_list_response(['gemma4:e4b']) + monkeypatch.setattr(canvigator_llm, '_make_client', lambda cloud=False: mock_client) + monkeypatch.setattr(canvigator_llm, 'selectFromList', lambda items, item_type="item": items[0]) + assert canvigator_llm.pickLocalGemmaModel() == 'gemma4:e4b' + + +class TestTranscribeAudio: + """`transcribe_audio` produces a transcript and optionally writes a sibling .txt for debugging.""" + + @staticmethod + def _mock_chat(transcript): + """Return a callable mimicking _chat_with_retry returning ``transcript``.""" + return lambda client, **kwargs: {'message': {'content': transcript}} + + def test_writes_sibling_txt_when_flag_set(self, tmp_path, monkeypatch): + """save_transcript=True writes a sibling .txt with the transcript content next to the audio.""" + import canvigator_llm + audio = tmp_path / "reply.wav" + audio.write_bytes(b"x" * 100) + monkeypatch.setattr(canvigator_llm, '_chat_with_retry', self._mock_chat('hello world')) + result = canvigator_llm.transcribe_audio(str(audio), MagicMock(), 'fake', save_transcript=True) + assert result == 'hello world' + txt = tmp_path / "reply.txt" + assert txt.exists() + assert txt.read_text() == 'hello world' + + def test_does_not_write_txt_when_flag_unset(self, tmp_path, monkeypatch): + """save_transcript=False leaves the replies dir untouched — no .txt produced.""" + import canvigator_llm + audio = tmp_path / "reply.wav" + audio.write_bytes(b"x" * 100) + monkeypatch.setattr(canvigator_llm, '_chat_with_retry', self._mock_chat('hello')) + canvigator_llm.transcribe_audio(str(audio), MagicMock(), 'fake', save_transcript=False) + assert not (tmp_path / "reply.txt").exists() + + def test_writes_empty_txt_for_empty_transcript(self, tmp_path, monkeypatch): + """An empty transcript still produces a zero-byte .txt — informative debug signal.""" + import canvigator_llm + audio = tmp_path / "reply.wav" + audio.write_bytes(b"x" * 100) + monkeypatch.setattr(canvigator_llm, '_chat_with_retry', self._mock_chat('')) + canvigator_llm.transcribe_audio(str(audio), MagicMock(), 'fake', save_transcript=True) + txt = tmp_path / "reply.txt" + assert txt.exists() + assert txt.stat().st_size == 0 + + def test_write_failure_logs_warning_and_still_returns_transcript(self, caplog, monkeypatch): + """If the .txt write fails (parent dir missing), the transcript is still returned.""" + import logging as _logging + import canvigator_llm + bad_path = "/nonexistent/path/reply.wav" + monkeypatch.setattr(canvigator_llm, '_chat_with_retry', self._mock_chat('still returns')) + with caplog.at_level(_logging.WARNING, logger='canvigator_llm'): + result = canvigator_llm.transcribe_audio(bad_path, MagicMock(), 'fake', save_transcript=True) + assert result == 'still returns' + assert any('Failed to write transcript' in r.message for r in caplog.records) + + def test_long_audio_is_chunked(self, tmp_path, monkeypatch): + """Audio longer than the encoder window is chunked; each chunk transcribed and joined.""" + import canvigator_llm + audio = tmp_path / "long.wav" + audio.write_bytes(b"x" * 100) + monkeypatch.setattr(canvigator_llm, '_probe_audio_duration_secs', lambda p: 75.0) + + def fake_ffmpeg(cmd, **kw): + Path(cmd[-1]).write_bytes(b"chunk") + return MagicMock(returncode=0, stdout=b'', stderr=b'') + monkeypatch.setattr(canvigator_llm.subprocess, 'run', fake_ffmpeg) + + call_count = [0] + + def fake_chat(client, **kwargs): + call_count[0] += 1 + return {'message': {'content': f'seg{call_count[0]}'}} + monkeypatch.setattr(canvigator_llm, '_chat_with_retry', fake_chat) + + result = canvigator_llm.transcribe_audio(str(audio), MagicMock(), 'fake') + # ceil(75 / 30) = 3 chunks + assert call_count[0] == 3 + assert result == 'seg1 seg2 seg3' + + def test_short_audio_single_pass(self, tmp_path, monkeypatch): + """Audio at or under the encoder window is transcribed in one call.""" + import canvigator_llm + audio = tmp_path / "short.wav" + audio.write_bytes(b"x" * 100) + monkeypatch.setattr(canvigator_llm, '_probe_audio_duration_secs', lambda p: 20.0) + call_count = [0] + + def fake_chat(client, **kwargs): + call_count[0] += 1 + return {'message': {'content': 'just one'}} + monkeypatch.setattr(canvigator_llm, '_chat_with_retry', fake_chat) + + result = canvigator_llm.transcribe_audio(str(audio), MagicMock(), 'fake') + assert call_count[0] == 1 + assert result == 'just one' + + def test_probe_failure_falls_back_to_single_pass(self, tmp_path, monkeypatch): + """If ffprobe returns 0.0, transcribe in one pass (matches prior behavior).""" + import canvigator_llm + audio = tmp_path / "x.wav" + audio.write_bytes(b"x" * 100) + monkeypatch.setattr(canvigator_llm, '_probe_audio_duration_secs', lambda p: 0.0) + monkeypatch.setattr(canvigator_llm, '_chat_with_retry', + lambda client, **kw: {'message': {'content': 'fallback'}}) + result = canvigator_llm.transcribe_audio(str(audio), MagicMock(), 'fake') + assert result == 'fallback'