Skip to content

Conversation

@amaslenn
Copy link
Contributor

Summary

Fix handling of a local path for docker container.

Test Plan

  1. CI (extended)
  2. Manual run.

Additional Notes

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 27, 2026

📝 Walkthrough

Walkthrough

This pull request updates the copyright year to 2026 and refines the SlurmInstaller's handling of DockerImage installations. A guard condition is added to prevent overwriting existing Path-based installed_path values when cache_docker_images_locally is enabled. A corresponding test validates this behavior.

Changes

Cohort / File(s) Summary
Installer logic and tests
src/cloudai/systems/slurm/slurm_installer.py, tests/test_slurm_installer.py
Copyright year bumped to 2026. Guard condition added to SlurmInstaller.mark_as_installed_one to preserve existing Path-based installed_path for DockerImages when cache_docker_images_locally is true. New test case validates that local container paths are not overwritten during installation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A guard stands tall, preventing paths that fall,
Copyright marches on to twenty-twenty-six,
Tests now verify with care, ensuring nothing breaks midair,
Local images stay put, no overwrite mix! 🎉

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title directly describes the main change: fixing handling of local Docker container paths, which is the core objective addressed in both code modifications.
Description check ✅ Passed The description relates to the changeset by stating the purpose as fixing handling of local paths for Docker containers, which matches the code changes.

✏️ 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.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 27, 2026

Greptile Overview

Greptile Summary

Fixed a bug in mark_as_installed_one where local Docker image paths were being overwritten with default cache paths. The fix adds a type check to preserve existing Path objects in installed_path.

  • Added condition and not isinstance(item.installed_path, Path) to line 158 in slurm_installer.py
  • Added comprehensive test case test_mark_as_installed_local_container that verifies local image paths are preserved
  • Updated copyright years to 2026 in both files

Confidence Score: 4/5

  • This PR is safe to merge with minor concerns about edge case handling
  • The fix addresses a real bug and includes a test case. However, the implementation checks the type of the returned property value rather than checking if the underlying field was explicitly set, which may not handle all theoretical edge cases (e.g., if installed_path were set to a string path). In practice, the codebase always uses Path objects for installed paths, so the fix works correctly for actual usage patterns.
  • Pay attention to src/cloudai/systems/slurm/slurm_installer.py line 158 - the type check could be made more robust

Important Files Changed

Filename Overview
src/cloudai/systems/slurm/slurm_installer.py Added check to preserve existing installed_path for Docker images when marking as installed; updated copyright year to 2026
tests/test_slurm_installer.py Added test case for local container path preservation; updated copyright year to 2026

Sequence Diagram

sequenceDiagram
    participant Client
    participant SlurmInstaller
    participant DockerImage
    participant SlurmSystem

    Client->>SlurmInstaller: mark_as_installed_one(docker_image)
    SlurmInstaller->>SlurmSystem: Check cache_docker_images_locally
    SlurmSystem-->>SlurmInstaller: Returns cache setting
    
    alt cache_docker_images_locally is True
        SlurmInstaller->>DockerImage: Check installed_path type
        
        alt installed_path is not a Path
            Note over SlurmInstaller,DockerImage: Path not set, use default cache location
            SlurmInstaller->>SlurmSystem: Get install_path
            SlurmSystem-->>SlurmInstaller: Returns install_path
            SlurmInstaller->>DockerImage: Set installed_path = install_path / cache_filename
        else installed_path is a Path
            Note over SlurmInstaller,DockerImage: Path already set (local image), preserve it
        end
    else cache_docker_images_locally is False
        Note over SlurmInstaller,DockerImage: No caching, installed_path remains as URL
    end
    
    SlurmInstaller-->>Client: Return InstallStatusResult(True)
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

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.

1 participant