Repository: skypilot-org/skypilot
Pull request: [Core] Execute setup when `--detach-setup` and no `run` section
<!-- Describe the changes in this PR -->

Fixes #4419


<!-- Describe the tests ran -->
<!-- Unit tests (tests/test_*.py) are part of GitHub CI; below are tests that launch on the cloud. -->

Tested (run the relevant ones):

- [ ] Code formatting: `bash format.sh`
- [ ] Any manual or new tests for this PR (please specify below)
  - [x] `sky launch -c test-setup -s test.yaml`
    ```
    resources:
      cloud: aws
      cpus: 2+
      region: us-east-1
    
    setup: hostname
    ```
- [ ] All smoke tests: `pytest tests/test_smoke.py` 
- [ ] Relevant individual smoke tests: `pytest tests/test_smoke.py::test_fill_in_the_name` 
- [ ] Backward compatibility tests: `conda deactivate; bash -i tests/backward_compatibility_tests.sh`


Problem statement:
[Core] `setup` will not run if you don't have a `run` section and `--detach-setup` is specified
<!-- Describe the bug report / feature request here -->




<!-- If relevant, fill in versioning info to help us troubleshoot -->
_Version & Commit info:_
* `sky -v`: PLEASE_FILL_IN
* `sky -c`: PLEASE_FILL_IN

Hints from issue discussion:
To reproduce:
```
# test.yaml
setup: hostname
```

```
sky launch -c test-detach --detach-setup test.yaml
```

The setup is not executed, due to: https://github.com/skypilot-org/skypilot/blob/23f9821bd165c8d154a44c3505a18b1e28cc0f48/sky/backends/cloud_vm_ray_backend.py#L3463-L3465

Commit message: Execute setup when --detach-setup and no run section

Patch to review:
diff --git a/sky/backends/cloud_vm_ray_backend.py b/sky/backends/cloud_vm_ray_backend.py
index 5682cf24586..19e76daa082 100644
--- a/sky/backends/cloud_vm_ray_backend.py
+++ b/sky/backends/cloud_vm_ray_backend.py
@@ -301,6 +301,8 @@ def add_prologue(self, job_id: int) -> None:
             )
             def get_or_fail(futures, pg) -> List[int]:
                 \"\"\"Wait for tasks, if any fails, cancel all unready.\"\"\"
+                if not futures:
+                    return []
                 returncodes = [1] * len(futures)
                 # Wait for 1 task to be ready.
                 ready = []
@@ -3460,15 +3462,23 @@ def _execute(
         Returns:
             Job id if the task is submitted to the cluster, None otherwise.
         """
-        if task.run is None:
+        if task.run is None and self._setup_cmd is None:
             logger.info('Run commands not specified or empty.')
             return None
-        # Check the task resources vs the cluster resources. Since `sky exec`
-        # will not run the provision and _check_existing_cluster
-        # We need to check ports here since sky.exec shouldn't change resources
-        valid_resource = self.check_resources_fit_cluster(handle,
-                                                          task,
-                                                          check_ports=True)
+        if task.run is None:
+            # If the task has no run command, we still need to execute the
+            # generated ray driver program to run the setup command in detached
+            # mode.
+            # In this case, we don't need to check the resources.
+            valid_resource = sky.Resources()
+        else:
+            # Check the task resources vs the cluster resources. Since
+            # `sky exec` will not run the provision and _check_existing_cluster
+            # We need to check ports here since sky.exec shouldn't change
+            # resources.
+            valid_resource = self.check_resources_fit_cluster(handle,
+                                                              task,
+                                                              check_ports=True)
         task_copy = copy.copy(task)
         # Handle multiple resources exec case.
         task_copy.set_resources(valid_resource)


Code review comments:
diff_hunk: @@ -3460,15 +3462,23 @@ def _execute(
         Returns:
             Job id if the task is submitted to the cluster, None otherwise.
         """
-        if task.run is None:
+        if task.run is None and self._setup_cmd is None:
             logger.info('Run commands not specified or empty.')
path: sky/backends/cloud_vm_ray_backend.py
text: @user1:
```suggestion
            logger.info('Both run and setup commands not specified or empty.')
```

@author:
Actually, it can be a bit confusing to show the new message here. Reason: there are several cases without run section specified:
1. A user specifies the setup command without `--detach-setup`, the setup will be executed correctly and the original message should be good to shown.
2. A user specifies setup command with `--detach-setup`, the setup will be executed  in detached mode without showing this message.
3. A user does not specify setup or run, the original message seems still fine.

In that case, how about we keep the original message?

@author:
Added some comments in the code : )

@user2:
An overall suggestion of adding too many comments: code should be self-explanatory and comments should be concise. For this specific case, I think it's fine to explain the cases when `self._setup_cmd is None` since it's less obvious (not just when there's no setup), but there's no need to explain why we don't write the code in certain way; that can further confuse the code reader

diff_hunk: @@ -3460,15 +3462,23 @@ def _execute(
         Returns:
             Job id if the task is submitted to the cluster, None otherwise.
         """
-        if task.run is None:
+        if task.run is None and self._setup_cmd is None:
             logger.info('Run commands not specified or empty.')
             return None
-        # Check the task resources vs the cluster resources. Since `sky exec`
-        # will not run the provision and _check_existing_cluster
-        # We need to check ports here since sky.exec shouldn't change resources
-        valid_resource = self.check_resources_fit_cluster(handle,
-                                                          task,
-                                                          check_ports=True)
+        if task.run is None:
+            # If the task has no run command, we still need to execute the
+            # generated ray driver program to run the setup command in detached
+            # mode.
+            # In this case, we don't need to check the resources.
+            valid_resource = sky.Resources()
path: sky/backends/cloud_vm_ray_backend.py
text: @user1:
If the task has gpu resources requirement, and the setup actually takes gpu to compute (e.g. some simple data preprocessing), should we keep the original resources requirement?

@author:
The setup script is not using the original task's resources anyway (we don't assign GPUs to setup in the generated program). That said, it will only increase the schedule time for the setup if we keep the task resources for setup only ones.

@author:
Added some comments : )