Fix a crash during dry-run for Dynamo scenario

[amaslenn]

Summary

Mark all installables as installed in dry-run + fix mark_as_installed() behavior for multiple items.

Fixes internal bug.

Test Plan

1. CI (extended)
2. Manual runs

Additional Notes

—

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

Installer now collects per-item results when marking items as installed and uses them to populate successful installs; CLI dry-run mode simulates installations by marking prepared items as installed; a test was added to verify two identical File instances are both marked installed with matching paths.

Changes

Cohort / File(s)	Summary
Installer result collection src/cloudai/_core/base_installer.py	mark_as_installed now captures return values from mark_as_installed_one into an install_results collection and calls _populate_successful_install with those results before returning the final InstallStatusResult.
Dry-run installation simulation src/cloudai/cli/handlers.py	handle_dry_run_and_run gained a branch for args.mode == "dry-run" that prepares installation items, invokes the installer to mark them as installed, and logs warnings if marking fails.
Multi-item installation test tests/test_base_installer.py	Added test_both_mark_as_installed in TestSuccessIsPopulated to assert two distinct File objects (same source) are both assigned a non-None installed_path and that those paths match after mark_as_installed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Check mark_as_installed change for correct collection handling, error propagation, and that _populate_successful_install receives expected result shapes.
• Verify dry-run branch does not alter existing dry-run semantics elsewhere and that logging/return paths are consistent.
• Ensure new test setup is deterministic and covers edge cases (identical sources, distinct instances).

Poem

🐰 I hopped through code, one, two, three,

Collected paths and shared them free.
Dry-run danced and marked the lot,
Two files found the same new spot.
A little test gave me a hop of glee.

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fix a crash during dry-run for Dynamo scenario' clearly describes the main change: fixing a crash during dry-run operations. This aligns with the PR's objective to address an internal bug by marking installables as installed in dry-run mode.
Description check	✅ Passed	The description relates to the changeset by summarizing the key changes: marking installables as installed in dry-run and fixing mark_as_installed() behavior for multiple items. It references the internal bug being fixed and includes a test plan.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch am/bug-4730612

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[greptile-apps]

Greptile Summary

This PR fixes a crash during dry-run for Dynamo scenarios by ensuring all installables have their _installed_path properly populated.

• Fixed mark_as_installed() in BaseInstaller to capture results and call _populate_successful_install(), ensuring duplicate installable objects get their paths set
• Added dry-run branch in handle_dry_run_and_run() to simulate installation by calling mark_as_installed()
• Added test case test_both_mark_as_installed to verify the fix works for equal-but-distinct File objects

Confidence Score: 5/5

• This PR is safe to merge - it's a targeted bug fix with proper test coverage.
• The changes are minimal and focused, fixing a specific bug in the mark_as_installed method. The fix follows the existing pattern used by install() and is_installed() methods. The new test case validates the fix works correctly.
• No files require special attention.

Important Files Changed

Filename	Overview
src/cloudai/_core/base_installer.py	Fixed mark_as_installed to properly populate _installed_path for duplicate items by capturing results and calling _populate_successful_install.
src/cloudai/cli/handlers.py	Added dry-run branch to simulate installation by calling mark_as_installed, ensuring installables have paths set during dry-run mode.
tests/test_base_installer.py	Added test case test_both_mark_as_installed to verify both equal-but-distinct File objects get their _installed_path set.

Sequence Diagram

sequenceDiagram
    participant CLI as handle_dry_run_and_run
    participant Installer as BaseInstaller
    participant Populate as _populate_successful_install

    CLI->>CLI: _check_installation()
    alt dry-run mode
        CLI->>Installer: mark_as_installed(installables)
        loop for each unique item
            Installer->>Installer: mark_as_installed_one(item)
            Note right of Installer: Sets _installed_path on unique item
        end
        Installer->>Populate: _populate_successful_install(items, results)
        loop for each item (with duplicates)
            Populate->>Installer: mark_as_installed_one(item)
            Note right of Installer: Sets _installed_path on duplicate items
        end
    end
    CLI->>CLI: Continue with dry-run execution

Loading

[greptile-apps]

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f5b2ceb and 93b8bc1.

📒 Files selected for processing (3)

• src/cloudai/_core/base_installer.py (1 hunks)
• src/cloudai/cli/handlers.py (1 hunks)
• tests/test_base_installer.py (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-12-16T19:47:41.994Z

Learnt from: amaslenn
Repo: NVIDIA/cloudai PR: 754
File: src/cloudai/_core/registry.py:226-234
Timestamp: 2025-12-16T19:47:41.994Z
Learning: In this repository, prefer expressing behavioral documentation through tests rather than docstrings. Tests act as living, verified documentation. Reserve docstrings for interfaces or high-level descriptions, and avoid duplicating behavior that is already covered by tests.

Applied to files:

• src/cloudai/cli/handlers.py
• tests/test_base_installer.py
• src/cloudai/_core/base_installer.py

🧬 Code graph analysis (3)

src/cloudai/cli/handlers.py (2)

src/cloudai/workloads/slurm_container/slurm_container.py (1)

• installables (47-48)

src/cloudai/_core/base_installer.py (1)

• mark_as_installed (261-278)

tests/test_base_installer.py (4)

tests/test_slurm_installer.py (1)

• installer (31-35)

src/cloudai/systems/slurm/slurm_installer.py (1)

• SlurmInstaller (39-367)

src/cloudai/_core/installables.py (1)

• File (149-167)

src/cloudai/_core/base_installer.py (1)

• mark_as_installed (261-278)

src/cloudai/_core/base_installer.py (3)

src/cloudai/_core/installables.py (1)

• Installable (25-32)

src/cloudai/_core/install_status_result.py (1)

• InstallStatusResult (22-52)

tests/test_base_installer.py (1)

• mark_as_installed_one (58-59)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Greptile Review

🔇 Additional comments (2)

src/cloudai/_core/base_installer.py (1)

271-276: LGTM! Proper fix for multiple identical items.

The method now correctly collects per-item results from mark_as_installed_one and passes them to _populate_successful_install, ensuring that multiple identical items (e.g., duplicate File instances) all receive their installed_path correctly. This aligns with the pattern used in install() and is_installed() methods.

tests/test_base_installer.py (1)

296-309: LGTM! Comprehensive test for the fix.

The test correctly validates that mark_as_installed handles multiple identical File instances properly, ensuring both receive their installed_path and share the same path. This directly supports the PR objective of fixing mark_as_installed behavior for multiple items.

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 93b8bc1 and 05b7165.

📒 Files selected for processing (1)

• src/cloudai/cli/handlers.py (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-12-16T19:47:41.994Z

Learnt from: amaslenn
Repo: NVIDIA/cloudai PR: 754
File: src/cloudai/_core/registry.py:226-234
Timestamp: 2025-12-16T19:47:41.994Z
Learning: In this repository, prefer expressing behavioral documentation through tests rather than docstrings. Tests act as living, verified documentation. Reserve docstrings for interfaces or high-level descriptions, and avoid duplicating behavior that is already covered by tests.

Applied to files:

• src/cloudai/cli/handlers.py

🧬 Code graph analysis (1)

src/cloudai/cli/handlers.py (1)

src/cloudai/_core/base_installer.py (1)

• mark_as_installed (261-278)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: Greptile Review
• GitHub Check: Run pytest (3.12)