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.

Let's revert this change to the original /tmp considering we have the check now?

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?

…l-path

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

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

Michaelvll

Thanks for the update @sethkimmel3! Sorry for the delay. The PR looks mostly good to me with some comments

Michaelvll · 2024-11-06T21:26:38Z

sky/utils/control_master_checks.py

+        return filesystem_type.lower() == '9p'
+
+    except CalledProcessError as e:
+        print(f'Error running "df" command: {e}')


Let's avoid printing directly to the output.

Suggested change

print(f'Error running "df" command: {e}')

logger.debug(f'Error running "df" command: {e}')

Michaelvll · 2024-11-06T21:29:01Z

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}'


Let's revert this change to the original /tmp considering we have the check now?

Michaelvll · 2024-11-06T21:29:35Z

sky/utils/command_runner.py

+        self.disable_control_master = (
+            control_master_checks.disable_control_master_checks() or
+            disable_control_master)


minor nit:

Suggested change

self.disable_control_master = (

control_master_checks.disable_control_master_checks() or

disable_control_master)

self.disable_control_master = (

disable_control_master or control_master_checks.disable_control_master_checks())

This may make it more efficient for the case disable_control_master is specified.

Michaelvll · 2024-11-07T21:51:58Z

sky/utils/control_master_checks.py

+    try:
+        result = subprocess_utils.run(['df', '-T', '/tmp'],
+                                      capture_output=True,
+                                      text=True)
+
+        if result.returncode != 0:
+            raise CalledProcessError(result.returncode, result.args,
+                                     result.stdout, result.stderr)
+
+        filesystem_info = result.stdout.strip().split('\n')[1]
+        filesystem_type = filesystem_info.split()[1]
+        return filesystem_type.lower() == '9p'
+
+    except CalledProcessError as e:
+        print(f'Error running "df" command: {e}')
+
+    return False


Making it more robust with:

Suggested change

try:

result = subprocess_utils.run(['df', '-T', '/tmp'],

capture_output=True,

text=True)

if result.returncode != 0:

raise CalledProcessError(result.returncode, result.args,

result.stdout, result.stderr)

filesystem_info = result.stdout.strip().split('\n')[1]

filesystem_type = filesystem_info.split()[1]

return filesystem_type.lower() == '9p'

except CalledProcessError as e:

print(f'Error running "df" command: {e}')

return False

result = subprocess_utils.run(['df', '-T', '/tmp'],

capture_output=True,

text=True)

if result.returncode != 0:

return False

filesystem_infos = result.stdout.strip().split('\n')

if len(filesystem_info) < 2:

return False

filesystem_types = filesystem_infos[1].split()

if len(filesystem_types) < 2:

return False

return filesystem_types[1].lower() == '9p'

Michaelvll · 2024-11-07T21:52:18Z

sky/utils/control_master_checks.py

+    return False
+
+
+def disable_control_master_checks() -> bool:


Suggested change

def disable_control_master_checks() -> bool:

@functools.lru_cache

def should_disable_control_master() -> bool:

We should add a lru_cache to avoid performance degradation due to many duplicate checks

Michaelvll · 2024-11-07T21:52:44Z

sky/utils/control_master_checks.py

+
+
+def disable_control_master_checks() -> bool:
+    """Disable ssh control master checks if the /tmp filesystem is 9p.


Suggested change

"""Disable ssh control master checks if the /tmp filesystem is 9p.

"""Whether disable ssh control master based on file system.

Michaelvll · 2024-11-07T21:55:25Z

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)


Add a comment for in what case this will not exist? Or should we remove this change for this PR?

sethkimmel3 · 2024-11-08T21:35:57Z

@Michaelvll - made the changes you suggested. Also had to make a small mod based on subprocess_utils based on my testing.

Not sure what's going on with the failing CI/CD test - have you seen that before?

Michaelvll

Thanks @sethkimmel3! LGTM. The CI/CD should be fixed if you merge the latest master.

Michaelvll · 2024-11-08T22:22:29Z

sky/utils/control_master_checks.py

+    return filesystem_types[1].lower() == '9p'
+
+
+@lru_cache


style: import module instead of functions

Suggested change

@lru_cache

@functools.lru_cache

Michaelvll · 2024-11-08T22:22:55Z

sky/utils/control_master_checks.py

@@ -0,0 +1,49 @@
+"""Utils to check if the ssh control master should be disabled."""


Would prefer renaming it to control_master_utils.py

Michaelvll · 2024-11-08T22:30:56Z

sky/utils/control_master_checks.py

+                                  capture_output=True,
+                                  text=True,
+                                  shell=None,
+                                  check=None,


Suggested change

check=None,

check=False,

sethkimmel3 · 2024-11-08T23:15:33Z

Looks like it worked now

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 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

Michaelvll reviewed Nov 7, 2024

View reviewed changes

sethkimmel3 added 5 commits November 8, 2024 14:08

updates from PR comments

e57714c

formatting

2ed568f

fix

29c4b46

dummy

75aa402

fix subprocess command

dfb501a

Michaelvll approved these changes Nov 8, 2024

View reviewed changes

sethkimmel3 added 2 commits November 8, 2024 16:58

more changes

0581e15

fix

68b005d

Michaelvll added this pull request to the merge queue Nov 9, 2024

Merged via the queue into skypilot-org:master with commit 409b92f Nov 9, 2024
20 checks passed

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 Nov 6, 2024

Michaelvll Sep 22, 2024

sethkimmel3 commented Oct 28, 2024

Michaelvll left a comment

Michaelvll Nov 6, 2024

Michaelvll Nov 6, 2024

Michaelvll Nov 6, 2024

Michaelvll Nov 7, 2024

Michaelvll Nov 7, 2024

Michaelvll Nov 7, 2024

Michaelvll Nov 7, 2024

sethkimmel3 commented Nov 8, 2024 •

edited

Loading

Michaelvll left a comment •

edited

Loading

Michaelvll Nov 8, 2024

Michaelvll Nov 8, 2024

Michaelvll Nov 8, 2024

sethkimmel3 commented Nov 8, 2024

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

	print(f'Error running "df" command: {e}')
	logger.debug(f'Error running "df" command: {e}')

	def disable_control_master_checks() -> bool:
	@functools.lru_cache
	def should_disable_control_master() -> bool:



		def disable_control_master_checks() -> bool:
		"""Disable ssh control master checks if the /tmp filesystem is 9p.

	"""Disable ssh control master checks if the /tmp filesystem is 9p.
	"""Whether disable ssh control master based on file system.

		@@ -0,0 +1,49 @@
		"""Utils to check if the ssh control master should be disabled."""

Mods to run from Modal #3908

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

Choose a reason for hiding this comment

sethkimmel3 commented Oct 28, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethkimmel3 commented Nov 8, 2024 • edited Loading

Michaelvll left a comment • edited Loading

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 Nov 8, 2024

sethkimmel3 commented Sep 4, 2024 •

edited

Loading

sethkimmel3 Oct 11, 2024 •

edited

Loading

sethkimmel3 Oct 11, 2024 •

edited

Loading

sethkimmel3 commented Nov 8, 2024 •

edited

Loading

Michaelvll left a comment •

edited

Loading