cli: hide traceback on interrupt, use os.execvp on supported systems #9805

skshetry · 2025-12-15T13:04:50Z

Closes #9460.

EDIT: This patch has been updated to use os.execvp on non-Windows systems. The discussion below is only applicable on Windows due to lack of POSIX exec*() functions.

This fix treats python -m lakefs ... as a convenient entry point, primarily intended for interactive use and limited workflows. So, this patch has two issues which I think is acceptable for that scenario:

In an interactive terminal, the kernel’s tty driver delivers SIGINT to all foreground processes. As a result, pressing Ctrl + C already sends SIGINT to both the Python interpreter and the lakefs binary. However, subprocess.run() waits for a short period, 0.25 seconds by default, before forcefully terminating the child process with SIGKILL. So, this may not allow the lakefs binary to exit gracefully.
If it is run in a non-interactive session, the lakefs binary never receives a signal, and it will get killed with a SIGKILL when the Python interpreter receives a SIGINT (after waiting for 0.25 second). The recommended way is to interrupt the binary rather than the lakefs module itself. So I think this is only a minor inconvenience.

Alternatively, we could SIG_IGN the interrupt, but wanted to propose something simpler.

Let me know if my understanding is incorrect.

skshetry · 2025-12-15T13:05:41Z

clients/python-wrapper/lakefs/__main__.py

    try:
-        print(f'running {binary_path}...')
-        proc = subprocess.run(
-            [binary_path] + args, check=False, env=os.environ)


check=False is the default. Same with env.

Looks like this is because of pylint.

skshetry · 2025-12-15T13:06:30Z

clients/python-wrapper/lakefs/__main__.py

-        proc = subprocess.run(
-            [binary_path] + args, check=False, env=os.environ)
-        return proc.returncode
-    except subprocess.CalledProcessError as e:


This does not get raised when check=False.

Good catch!

skshetry · 2025-12-15T13:08:14Z

clients/python-wrapper/lakefs/__main__.py

-    if not binary_path:
-        raise RuntimeError("binary not found")


Removed this if condition from here and moved to find_or_download_binary, because the typehint says it cannot return None:

lakeFS/clients/python-wrapper/lakefs/__main__.py

Line 166 in b764484

def find_or_download_binary(binary_name: str) -> str:

I think you're right that the typing is wrong! find_or_download_binary may easily return null: it calls _find_binary and fails, then download_binaries does something wrong, and then _find_binary fails again. For instance, if I delete the downloaded lakefs right after you download the binary, then there is no binary to be found.

So please keep this check; consider correcting the types instead. Or correct find_or_download_binary to do check the second return. Or "cast" it there with an appropriate comment that f_o_d_b always returns an actual str.

Or correct find_or_download_binary to do check the second return.

yes, that's what I have done. find_or_download_binary raises if they are falsy values.

lakeFS/clients/python-wrapper/lakefs/__main__.py

Lines 166 to 178 in 69c9b01

def find_or_download_binary(binary_name: str) -> str:

'''

Find the binary in PATH or ~/.lakefs/bin,

or download it if not found

Returns the path to the binary

'''

binary_path = _find_binary(binary_name)

if not binary_path:

_download_binaries()

binary_path = _find_binary(binary_name)

if not binary_path:

raise RuntimeError("binary not found")

return binary_path

skshetry · 2025-12-15T13:39:34Z

clients/python-wrapper/lakefs/__main__.py

-        return proc.returncode
-    except subprocess.CalledProcessError as e:
-        raise RuntimeError(f"Error executing {binary_name}: {e}") from e
+        proc = subprocess.run([binary_path, *args], check=False)


We could also use os.execvp on posix systems.

I see that ruff does that, which also has a python wrapper executing rust binary.

https://github.com/astral-sh/ruff/blob/d08e41417971f1d05b9daa75f794536a1dd4bedf/python/ruff/__main__.py#L88

This would be much much better. It will avoid the whole 0.25s debacle - I was shocked by your link to the code showing that this was hard-coded.¹

Footnotes

The code you showed appears to violate these Zen of Python principles:

Beautiful is better than ugly.

Explicit is better than implicit.

Special cases aren't special enough to break the rules.

In the face of ambiguity, refuse the temptation to guess.

If the implementation is hard to explain, it's a bad idea.

Breaking 5 out of 19 aphorisms is... impressive. ↩

If lakefs terminates immediately, it does not wait all 0.25s. A bigger problem is what follows after the _wait(), a SIGKILL, preventing a graceful termination.

arielshaqed

Thanks! Really solid work. But the 0.25s thing is a deal-breaker. Much safer and simpler just to call sys.exec*. Please do that; sorry.

arielshaqed · 2025-12-15T13:30:52Z

clients/python-wrapper/lakefs/__main__.py

-    if not binary_path:
-        raise RuntimeError("binary not found")


I think you're right that the typing is wrong! find_or_download_binary may easily return null: it calls _find_binary and fails, then download_binaries does something wrong, and then _find_binary fails again. For instance, if I delete the downloaded lakefs right after you download the binary, then there is no binary to be found.

So please keep this check; consider correcting the types instead. Or correct find_or_download_binary to do check the second return. Or "cast" it there with an appropriate comment that f_o_d_b always returns an actual str.

arielshaqed · 2025-12-15T13:34:17Z

clients/python-wrapper/lakefs/__main__.py

-        proc = subprocess.run(
-            [binary_path] + args, check=False, env=os.environ)
-        return proc.returncode
-    except subprocess.CalledProcessError as e:


Good catch!

arielshaqed · 2025-12-15T13:57:28Z

clients/python-wrapper/lakefs/__main__.py

-        return proc.returncode
-    except subprocess.CalledProcessError as e:
-        raise RuntimeError(f"Error executing {binary_name}: {e}") from e
+        proc = subprocess.run([binary_path, *args], check=False)


This would be much much better. It will avoid the whole 0.25s debacle - I was shocked by your link to the code showing that this was hard-coded.¹

Footnotes

The code you showed appears to violate these Zen of Python principles:

Beautiful is better than ugly.

Explicit is better than implicit.

Special cases aren't special enough to break the rules.

In the face of ambiguity, refuse the temptation to guess.

If the implementation is hard to explain, it's a bad idea.

Breaking 5 out of 19 aphorisms is... impressive. ↩

arielshaqed

Thanks! Better and improves typing.

arielshaqed · 2025-12-16T10:09:10Z

clients/python-wrapper/lakefs/__main__.py

+    if sys.platform == 'win32':
+        try:
+            proc = subprocess.run([binary_path, *args], check=False)
+        except KeyboardInterrupt:
+            sys.exit(1)
+        sys.exit(proc.returncode)


Poor Windows users. But since it's unclear how to improve matters, and since the existing experience is certainly more than good enough, I see no reason to add work here.

arielshaqed · 2025-12-16T10:10:06Z

clients/python-wrapper/lakefs/__main__.py



-def cli_run() -> int:
+def run(binary_name: Literal['lakefs', 'lakectl'], args: Optional[list[str]] = None) -> NoReturn:


Nit: This function will work perfectly well with any binary downloadable from our repo. I see no reason to limit its typing beyond str.

Not really, as we only extract lakefs and lakectl.

lakeFS/clients/python-wrapper/lakefs/__main__.py

Lines 102 to 103 in 7aaa374

tar.extract('lakefs', target_dir, filter='data')

tar.extract('lakectl', target_dir, filter='data')

But I get your point. Fixed in 01be1b3.

github-actions bot added the area/sdk/python label Dec 15, 2025

skshetry commented Dec 15, 2025

View reviewed changes

lakefs: handle KeyboardInterrupt when executing the binary

69c9b01

skshetry force-pushed the fix/lakefs-bin-exec-9460 branch from b764484 to 69c9b01 Compare December 15, 2025 13:09

skshetry requested a review from a team December 15, 2025 13:15

arielshaqed requested review from arielshaqed and removed request for a team December 15, 2025 13:26

skshetry commented Dec 15, 2025

View reviewed changes

arielshaqed requested changes Dec 15, 2025

View reviewed changes

use os.execvp on posix systems

885db9c

skshetry requested a review from arielshaqed December 16, 2025 05:12

skshetry added 2 commits December 16, 2025 11:24

DRY code

6e649f2

fix docstring

7aaa374

skshetry changed the title ~~lakefs: handle KeyboardInterrupt when executing the binary~~ cli: hide traceback on interrupt, use os.execvp on supported systems Dec 16, 2025

arielshaqed approved these changes Dec 16, 2025

View reviewed changes

adjust typehint to allow running arbitrary executable

01be1b3

skshetry force-pushed the fix/lakefs-bin-exec-9460 branch from 01f2d28 to 01be1b3 Compare December 16, 2025 12:29

	def find_or_download_binary(binary_name: str) -> str:
	'''
	Find the binary in PATH or ~/.lakefs/bin,
	or download it if not found
	Returns the path to the binary
	'''
	binary_path = _find_binary(binary_name)
	if not binary_path:
	_download_binaries()
	binary_path = _find_binary(binary_name)
	if not binary_path:
	raise RuntimeError("binary not found")
	return binary_path



		def cli_run() -> int:
		def run(binary_name: Literal['lakefs', 'lakectl'], args: Optional[list[str]] = None) -> NoReturn:

	tar.extract('lakefs', target_dir, filter='data')
	tar.extract('lakectl', target_dir, filter='data')

cli: hide traceback on interrupt, use os.execvp on supported systems #9805

Are you sure you want to change the base?

cli: hide traceback on interrupt, use os.execvp on supported systems #9805

Uh oh!

Conversation

skshetry commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skshetry Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skshetry Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skshetry Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielshaqed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

arielshaqed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skshetry Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

skshetry commented Dec 15, 2025 •

edited

Loading

skshetry Dec 15, 2025 •

edited

Loading

skshetry Dec 15, 2025 •

edited

Loading

skshetry Dec 15, 2025 •

edited

Loading

skshetry Dec 16, 2025 •

edited

Loading