Feat/jobstats fallback - #94
Conversation
…able Implements a best-effort GPU metrics recovery mechanism that activates when Slurm job records lack AdminComment data (e.g., when jobstats' optional mirror_to_admin_comment flag is disabled). Changes: - Added _check_jobstats_available() to detect jobstats binary and -b flag support - Added augment_with_jobstats() to batch jobstats queries for missing GPU metrics - Integrated fallback into console.py with --no-jobstats-fallback CLI flag - Added comprehensive test coverage (14 new tests, all passing) Design: - Batching: All missing jobs sent to jobstats in single subprocess call - Opt-out: Fallback enabled by default if jobstats is available - Graceful: Partial failures handled, subprocess errors caught - Transparent: Existing parsing logic reused without modification - Array jobs: Efficiently handled with single batched query per array Motivation: Jobstats is migrating GPU metrics to Prometheus + MariaDB (PR#55). The optional mirror_to_admin_comment config preserves AdminComment data. Without it, reportseff's GPU columns would show "---" (broken functionality). This fallback enables automatic GPU metric recovery when jobstats is available. Related: PrincetonUniversity/jobstats#55 Testing: All 136 tests pass (122 existing + 14 new)
|
@troycomi Sorry, I noticed the coverage test is not passing. |
…R was submitted (poe tests).
…e a versions of click that were triggering an error during the audit as they have been reported as unsafe. The versions are marked as so both in the pyproject.toml and the uv.lock project files.
…exceed the recommended length.
|
Thanks for your patience, I'm getting back from vacation and slowly catching back up. This looks like a solid PR that address the jobstats issue I was hoping to just ignore! A few changes I'd prefer but am open to discussion. I think the option should probably be a pair, like I would also like it to be off by default. My concern is cases when jobstats is available but writes SHORT to the admin comment, causing extra overhead. Is that possible? Perhaps an alternative is to only enable that when gpu or multinode reporting is also set? Curious on your thoughts here. |
|
Hello @troycomi,👋 I understand your concerns although perhaps not fully. After reading your comment I started thinking that instead of second guessing the --read-gpu-perf:
|
|
That could be a nice interface. Let me think out loud for a minute.
No rush, enjoy your vacation! |
2 participants
This feature fixes a silent break that occurs in the more recent versions of
jobstats. Originally,reportseffuses theAdminCommentfield insacctto obtain the job's GPU performance metrics (GpuEff, GpuMemEff). But that field has been removed in favor of using an external database. Thejobstatsproject already provides a fix which involves mirroring that information in slurm's dbAdminCommentfield. I'm also the one who proposed that fix.In some
jobstatscluster deployments, the cluster admin might decide not to apply the fix. In those cases this pull request is providing a last resort fallback, to obtain the GPU metrics usingjobstatspublic API instead. I've first got the idea for this workaround from a developer's comment on thejobstatsissue discussion, where the silent break was highlighted.It's important to stress that the
AdminCommentmirror is in my opinion and from a reportseff user's egoistic perspective, the preferred and faster fix. In any case, reportseff users can decide to disable the fallback by passing a command line option to avoid the additional time penalty.I've used a Coding Agent website (claude.ai/Sonnet 4.6) to engineer and implement this feature.