Skip to content

Conversation

@amaslenn
Copy link
Contributor

Summary

Make tests more stable on systems with slurm binaries by disabling real system check.

Test Plan

  1. CI

Additional Notes

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 21, 2026

📝 Walkthrough

Walkthrough

Two test files were modified to enable GPU directives caching by setting supports_gpu_directives_cache = True on SlurmSystem instances; each change is a single-line configuration in the test setups.

Changes

Cohort / File(s) Summary
GPU directives caching configuration
tests/test_acceptance.py, tests/test_single_sbatch_runner.py
Set SlurmSystem.supports_gpu_directives_cache = True in test setups to enable GPU directives caching at runtime.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 I flipped a flag and cached the run,
GPUs hum softly, tasks now done,
Two tests cheer, a tiny tweak,
Speed and order hide and peek,
Nibbles of code — a rabbit's fun.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: improving test stability on systems with Slurm binaries by disabling real system checks.
Description check ✅ Passed The description is directly related to the changeset, explaining that the PR disables real system checks to improve test stability on systems with Slurm binaries.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/test_single_sbatch_runner.py (1)

1-3: Fix header year to satisfy CI policy (Line 2).

CI reports a header policy failure: expected 2025-2026, found 2025. Update the copyright line to unblock CI.

🛠️ Proposed fix
-# Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+# Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 21, 2026

Greptile Summary

This PR prevents test instability on systems with Slurm binaries by pre-setting the supports_gpu_directives_cache property to True in two test files.

Key changes:

  • Set slurm_system.supports_gpu_directives_cache = True in test_sbatch_generation (test_acceptance.py:528)
  • Set runner.system.supports_gpu_directives_cache = True in test_sbatch_system_fields (test_single_sbatch_runner.py:103)
  • Updated copyright year range to 2025-2026 in test_single_sbatch_runner.py

How it works:
The supports_gpu_directives property checks if the system supports GPU directives by executing scontrol show config when the cache is not set. By pre-setting the cache in tests, the property returns the cached value immediately without executing real system commands, preventing tests from failing on systems where Slurm binaries are present but configured differently than expected.

Confidence Score: 5/5

  • This PR is safe to merge with no risk
  • The changes are minimal, focused, and follow established patterns in the codebase. The fix correctly addresses test instability by preventing unwanted system calls during testing. The same pattern is already used in test_sbatch_default (line 75 of test_single_sbatch_runner.py), confirming this is the standard approach. The copyright update is routine maintenance. No logic changes, no new functionality, and the changes are isolated to test files only.
  • No files require special attention

Important Files Changed

Filename Overview
tests/test_acceptance.py Set supports_gpu_directives_cache = True to prevent real system check during tests
tests/test_single_sbatch_runner.py Set supports_gpu_directives_cache = True and updated copyright to 2025-2026

Sequence Diagram

sequenceDiagram
    participant Test as Test Suite
    participant SlurmSystem
    participant Cache as supports_gpu_directives_cache
    participant System as Real System (scontrol)

    Note over Test,System: Before this PR
    Test->>SlurmSystem: Access supports_gpu_directives property
    SlurmSystem->>Cache: Check cache value
    Cache-->>SlurmSystem: None (not set)
    SlurmSystem->>System: Execute 'scontrol show config'
    System-->>SlurmSystem: Return system output
    SlurmSystem->>Cache: Store result in cache
    SlurmSystem-->>Test: Return GPU support status

    Note over Test,System: After this PR
    Test->>Cache: Set supports_gpu_directives_cache = True
    Test->>SlurmSystem: Access supports_gpu_directives property
    SlurmSystem->>Cache: Check cache value
    Cache-->>SlurmSystem: True (pre-set by test)
    SlurmSystem-->>Test: Return True (no system call)
Loading

Copy link

@alexmanle alexmanle left a comment

Choose a reason for hiding this comment

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

Looks good!

@amaslenn amaslenn merged commit a55570b into main Jan 21, 2026
6 checks passed
@amaslenn amaslenn deleted the am/stable-tests branch January 21, 2026 15:40
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.

2 participants