Mods to run from Modal #3908

sethkimmel3 · 2024-09-04T04:45:41Z

The small changes in this PR were necessary to successfully deploy Skypilot from Modal. I believe that the root cause issue is a specialized filesystem that Modal uses and mounts to containers for faster provisioning and startup time.

I've marked it as a draft for now, as I'm assuming we want to let a user add a configuration, perhaps to the config.yaml advanced configurations to disable the ssh control master. There is an example in backend_utils.py that disables it for kubernetes, but I don't see a more general pattern to allow such an override:

skypilot/sky/backends/backend_utils.py

Line 1273 in e3b9763

credentials['disable_control_master'] = True

I believe the other change is relatively benign, but I'd be open to other suggestions to fix this too.

Tested (run the relevant ones):

Code formatting: bash format.sh
Any manual or new tests for this PR (please specify below)
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

sethkimmel3 · 2024-09-04T16:21:05Z

@Michaelvll could you provide some guidance on how you'd like to integrate these changes if you're open to accepting them?

…l-path

Michaelvll

Thanks for submitting this PR @sethkimmel3! We should probably not remove the control master for all the other cases, as that can significantly increase the time for executing commands on the remote cluster.

Michaelvll · 2024-09-10T01:19:17Z

sky/backends/wheel_utils.py

+        if not os.path.exists(os.path.join(wheel_dir, os.path.basename(wheel_path))):
+            shutil.move(str(wheel_path), wheel_dir)


This seems indicating that the temp folder we are using in the code is not available for writing wheels to. Can we debug a bit for why the /tmp folder is not working and whether it will work with tempfile.gettempdir() instead of using the hardcoded /tmp.

When I revert to the original code, the error I'm getting is: shutil.Error: Destination path '/root/.sky/wheels/0b62cd7da6552f20e52d8a83687dd835/skypilot-1.0.0.dev0-py3-none-any.whl' already exists. So I think it's actually that the wheel is persisting, and shutil.move fails to overwrite if it exists. Is it atypical that the wheel would already be found at the destination?

I think there's a number of ways to fix this that are relatively harmless and idempotent. This could be checking if the file already exists and skipping (as I did here), or running a shutil method that overwrites instead. Just let me know what you're preference is.

Hmm, this is weird. The original code's destination, wheel_dir, is a directory (supposedly to be /root/.sky/wheels/0b62cd7da6552f20e52d8a83687dd835) but the error message you shown shutil.Error: Destination path '/root/.sky/wheels/0b62cd7da6552f20e52d8a83687dd835/skypilot-1.0.0.dev0-py3-none-any.whl' already exists indicates the wheel_dir somehow becomes the target file.

IIUC, if the destination is a dir, shutil should correctly overwrite the file in it. Could you help check if that is the case in your case?

Here's an example printout of the wheel_path, and wheel_dir:
Wheel path: /tmp/tmp1ca8zhil/skypilot-1.0.0.dev0-py3-none-any.whl
Wheel dir: /root/.sky/wheels/6b3f8d33bac9d114a388c31be0e1998e

I think it's possible that the 9p filesystem is again doing something odd here. Also from observation, it starts to fail the second time (not first time) the wheel hash changes. I probably need to do more digging here myself but let me know if that's helpful.

So the reason this is failing seems simple. It turns out that shutil.move fails during a move across two filesystems. Since I'm moving the wheel from Modal's tmp mount to my own .sky mount, this makes sense. It seems like the best option is to use shutil.copy2 or check if the wheel already exists before attempting to move.

…l-path

sethkimmel3 · 2024-09-20T18:23:13Z

@Michaelvll I agree changing the default behavior of the control master isn't a good idea. However, modifying the behavior seems critical in this case. Do you see a good option to parameterize this, potentially in config.yaml, so the user can change the behavior if necessary?

sethkimmel3 · 2024-09-21T00:06:47Z

Another finding @Michaelvll - if I set the ssh control path to f'/dev/shm/skypilot_ssh_{user_hash}/{ssh_control_filename}'rather than f'/tmp/skypilot_ssh_{user_hash}/{ssh_control_filename}', I can leave the default control master behavior.

I believe this is due to Modal using a 9p filesystem under the hood, which seems to not interface well with the symlink behavior required to use ssh control master. Again, I'm not sure if this would be an acceptable change, or something that could be configurable by the user.

Michaelvll · 2024-09-21T23:48:36Z

sky/utils/command_runner.py

@@ -401,6 +402,7 @@ def __init__(
        ssh_proxy_command: Optional[str] = None,
        docker_user: Optional[str] = None,
        disable_control_master: Optional[bool] = False,
+        # disable_control_master: Optional[bool] = True,


Suggested change

# disable_control_master: Optional[bool] = True,

Let's remove this unused comment : )

Michaelvll · 2024-09-22T00:35:34Z

sky/utils/command_runner.py

@@ -42,7 +43,7 @@ def _ssh_control_path(ssh_control_filename: Optional[str]) -> Optional[str]:
    if ssh_control_filename is None:
        return None
    user_hash = common_utils.get_user_hash()
-    path = f'/tmp/skypilot_ssh_{user_hash}/{ssh_control_filename}'
+    path = f'/dev/shm/skypilot_ssh_{user_hash}/{ssh_control_filename}'


Is it possible to use tempfile.gettempdir() instead of /dev/shm/ to be more general?

Looks like tempfile.gettempdir() returns /tmp when run from a Modal container.

It seems control master is only supported on some specific file system types and that seems to be why it fails on modal's container. I am thinking that we should check the file system type to decide whether we should use control master. Changing the path to /dev/shm/ feels a bit losing generality for other systems, for example

def _file_system_allow_ssh_control_master() -> bool: file_system_type = None partitions = psutil.disk_partitions() for partition in partitions: if '/' in partition.mountpoint and file_system_type is None: file_system_type = partition.fstype if '/tmp' in partition.mountpoint: file_system_type = partition.fstype break # Allow unknown file system types, as well as ext4, xfs, btrfs, and apfs. if file_system_type in ['ext4', 'xfs', 'btrfs', 'apfs', None]: return True return False

I think this makes sense. I'll check on my end to make sure it detects the 9p filesystem correctly. My only concern is losing some performance benefits of the ssh control master.

It doesn't look like your function captures the 9p filesystem for /tmp. Since it's the only erroneous case that's specifically known as of now, would something like this work?

import subprocess def is_tmp_9p_filesystem(): try: # Use 'df' command to get filesystem information for /tmp result = subprocess.run(['df', '-T', '/tmp'], capture_output=True, text=True) if result.returncode != 0: raise subprocess.CalledProcessError(result.returncode, result.args, result.stdout, result.stderr) # Split the output into lines and get the second line (which contains the filesystem info) filesystem_info = result.stdout.strip().split('\n')[1] # The second column of this line is the filesystem type filesystem_type = filesystem_info.split()[1] # Check if the filesystem type is '9p' return filesystem_type.lower() == '9p' except subprocess.CalledProcessError as e: print(f"Error running 'df' command: {e}") except Exception as e: print(f"Unexpected error: {e}") return False

If it's True, then we disable ssh_control_master.

Michaelvll · 2024-09-22T00:41:31Z

sky/backends/wheel_utils.py

+        if not os.path.exists(os.path.join(wheel_dir, os.path.basename(wheel_path))):
+            shutil.move(str(wheel_path), wheel_dir)


Hmm, this is weird. The original code's destination, wheel_dir, is a directory (supposedly to be /root/.sky/wheels/0b62cd7da6552f20e52d8a83687dd835) but the error message you shown shutil.Error: Destination path '/root/.sky/wheels/0b62cd7da6552f20e52d8a83687dd835/skypilot-1.0.0.dev0-py3-none-any.whl' already exists indicates the wheel_dir somehow becomes the target file.

IIUC, if the destination is a dir, shutil should correctly overwrite the file in it. Could you help check if that is the case in your case?

sethkimmel3 · 2024-09-23T17:34:19Z

@Michaelvll here's where things stand:

To solve the ssh control path issue, we can do one of two things - use a tmpfs type directory; in Modal's case /dev/shm/, as a 9p filesystem seems incompatible with using ssh control paths. So it seems viable to let a user choose an ssh control path directory in config.yaml, or let a user control whether to disable the ssh control master.
The second issue stems from trying to move across filesystems. We can use shutil.copy2 or simply check in advance whether the file already exists so shutil.move doesn't fail.

…l-path

Michaelvll · 2024-10-11T04:00:14Z

sky/backends/wheel_utils.py

+        shutil.copy2(str(wheel_path), wheel_dir)
+        # if not os.path.exists(os.path.join(wheel_dir, os.path.basename(wheel_path))):
+        #     shutil.move(str(wheel_path), wheel_dir)


Is this change mainly caused by sharing the ~/.sky folder across multiple functions?

I believe it's because we're moving the wheel from a 9p to tmpfs type filesystem (or vice versa).

Michaelvll · 2024-10-11T04:05:44Z

sky/utils/command_runner.py

@@ -42,7 +43,7 @@ def _ssh_control_path(ssh_control_filename: Optional[str]) -> Optional[str]:
    if ssh_control_filename is None:
        return None
    user_hash = common_utils.get_user_hash()
-    path = f'/tmp/skypilot_ssh_{user_hash}/{ssh_control_filename}'
+    path = f'/dev/shm/skypilot_ssh_{user_hash}/{ssh_control_filename}'


It seems control master is only supported on some specific file system types and that seems to be why it fails on modal's container. I am thinking that we should check the file system type to decide whether we should use control master. Changing the path to /dev/shm/ feels a bit losing generality for other systems, for example

def _file_system_allow_ssh_control_master() -> bool: file_system_type = None partitions = psutil.disk_partitions() for partition in partitions: if '/' in partition.mountpoint and file_system_type is None: file_system_type = partition.fstype if '/tmp' in partition.mountpoint: file_system_type = partition.fstype break # Allow unknown file system types, as well as ext4, xfs, btrfs, and apfs. if file_system_type in ['ext4', 'xfs', 'btrfs', 'apfs', None]: return True return False

…l-path

sethkimmel3 · 2024-10-28T01:45:14Z

@Michaelvll - I proposed an implementation here. Let me know what you think!

sethkimmel3 added 4 commits September 2, 2024 21:19

Update command_runner.py

d69848a

Update command_runner.py

df30ac7

Update wheel_utils.py

8d93356

Update command_runner.py

f27df54

sethkimmel3 changed the title ~~Sethkimmel3 modify ssh control path~~ Mods to run from Modal Sep 4, 2024

sethkimmel3 added 2 commits September 5, 2024 12:40

Merge branch 'skypilot-org:master' into sethkimmel3-modify-ssh-contro…

5be67a7

…l-path

Merge branch 'skypilot-org:master' into sethkimmel3-modify-ssh-contro…

df2bc33

…l-path

Michaelvll reviewed Sep 10, 2024

View reviewed changes

sethkimmel3 added 2 commits September 19, 2024 20:06

Merge branch 'skypilot-org:master' into sethkimmel3-modify-ssh-contro…

b69c583

…l-path

Update wheel_utils.py

7c57122

sethkimmel3 added 9 commits September 20, 2024 11:44

Update wheel_utils.py

dab6ab3

Update command_runner.py

d1b7b67

Update command_runner.py

fb1b621

Update command_runner.py

9395450

Update command_runner.py

03d9c32

Update command_runner.py

1805834

Update command_runner.py

c60a5b5

Update command_runner.py

a24b2df

Update command_runner.py

5de6aee

Michaelvll reviewed Sep 22, 2024

View reviewed changes

sethkimmel3 added 7 commits September 21, 2024 18:33

Update command_runner.py

e278cab

Update command_runner.py

272f6ef

Update wheel_utils.py

f234c5b

Update wheel_utils.py

0ff8ba1

Update wheel_utils.py

4246672

Update command_runner.py

ed4570f

Update wheel_utils.py

6ab5f31

sethkimmel3 added 5 commits September 23, 2024 09:40

Update wheel_utils.py

0c8205f

Update wheel_utils.py

c4c7c2c

Update wheel_utils.py

a9fff3c

Update wheel_utils.py

ff9b36a

Update wheel_utils.py

7aa32ee

Merge branch 'skypilot-org:master' into sethkimmel3-modify-ssh-contro…

a22c884

…l-path

Michaelvll reviewed Oct 11, 2024

View reviewed changes

sethkimmel3 requested a review from Michaelvll October 16, 2024 18:04

sethkimmel3 added 6 commits October 17, 2024 13:42

Merge branch 'skypilot-org:master' into sethkimmel3-modify-ssh-contro…

48fe52b

…l-path

Create control_master_checks.py

6120952

Update command_runner.py

63c7eb1

Update wheel_utils.py

36dcbcb

Merge branch 'skypilot-org:master' into sethkimmel3-modify-ssh-contro…

f288ebe

…l-path

fix import

5d04ba8

sethkimmel3 marked this pull request as ready for review October 27, 2024 23:51

sethkimmel3 added 7 commits October 27, 2024 16:59

fix some pylint stuff

e1793be

more pylint

468cf51

more pylint

742875a

more pylint

3613c38

yapf'

7a1bdc3

reorder imports

dfd6e33

fix

0386507

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mods to run from Modal #3908

Mods to run from Modal #3908

sethkimmel3 commented Sep 4, 2024 •

edited

Loading

sethkimmel3 commented Sep 4, 2024

Michaelvll left a comment

Michaelvll Sep 10, 2024

sethkimmel3 Sep 20, 2024

Michaelvll Sep 22, 2024

sethkimmel3 Sep 22, 2024

sethkimmel3 Sep 23, 2024

sethkimmel3 commented Sep 20, 2024

sethkimmel3 commented Sep 21, 2024

Michaelvll Sep 21, 2024

Michaelvll Sep 22, 2024

sethkimmel3 Sep 22, 2024

Michaelvll Oct 11, 2024

sethkimmel3 Oct 11, 2024 •

edited

Loading

sethkimmel3 Oct 11, 2024 •

edited

Loading

Michaelvll Sep 22, 2024

sethkimmel3 commented Sep 23, 2024

Michaelvll Oct 11, 2024

sethkimmel3 Oct 11, 2024

Michaelvll Oct 11, 2024

sethkimmel3 commented Oct 28, 2024

		if not os.path.exists(os.path.join(wheel_dir, os.path.basename(wheel_path))):
		shutil.move(str(wheel_path), wheel_dir)

Mods to run from Modal #3908

Are you sure you want to change the base?

Mods to run from Modal #3908

Conversation

sethkimmel3 commented Sep 4, 2024 • edited Loading

sethkimmel3 commented Sep 4, 2024

Michaelvll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethkimmel3 commented Sep 20, 2024

sethkimmel3 commented Sep 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethkimmel3 Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

sethkimmel3 Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethkimmel3 commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethkimmel3 commented Oct 28, 2024

sethkimmel3 commented Sep 4, 2024 •

edited

Loading

sethkimmel3 Oct 11, 2024 •

edited

Loading

sethkimmel3 Oct 11, 2024 •

edited

Loading