-
Notifications
You must be signed in to change notification settings - Fork 3
PR4: SHM leak fixes, sandbox detection, 0.9.1 release prep #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
63e874f
cc3f753
e187670
495c7f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||||||
| import argparse | ||||||||||||||||
| import asyncio | ||||||||||||||||
| import inspect | ||||||||||||||||
| import logging | ||||||||||||||||
| import os | ||||||||||||||||
| import sys | ||||||||||||||||
|
|
@@ -9,6 +10,7 @@ | |||||||||||||||
| from shared import DatabaseSingleton, ExampleExtensionBase | ||||||||||||||||
|
|
||||||||||||||||
| import pyisolate | ||||||||||||||||
| from pyisolate._internal.sandbox_detect import detect_sandbox_capability | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Example imports from internal
Option: export it publiclyIn from ._internal.tensor_serializer import flush_tensor_keeper, purge_orphan_sender_shm_files
+from ._internal.sandbox_detect import detect_sandbox_capability
from .config import ExtensionConfig, ExtensionManagerConfig, SandboxMode __all__ = [
...
+ "detect_sandbox_capability",
"register_adapter",Then in -from pyisolate._internal.sandbox_detect import detect_sandbox_capability
+from pyisolate import detect_sandbox_capability🤖 Prompt for AI Agents |
||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| # ANSI color codes for terminal output (using 256-color mode for better compatibility) | ||||||||||||||||
|
|
@@ -47,6 +49,16 @@ async def async_main(): | |||||||||||||||
| config = pyisolate.ExtensionManagerConfig(venv_root_path=os.path.join(base_path, "extension-venvs")) | ||||||||||||||||
| manager = pyisolate.ExtensionManager(ExampleExtensionBase, config) | ||||||||||||||||
|
|
||||||||||||||||
| sandbox_mode = pyisolate.SandboxMode.REQUIRED | ||||||||||||||||
| if sys.platform == "linux": | ||||||||||||||||
| cap = detect_sandbox_capability() | ||||||||||||||||
| if not cap.available: | ||||||||||||||||
| sandbox_mode = pyisolate.SandboxMode.DISABLED | ||||||||||||||||
| logger.warning( | ||||||||||||||||
| "Sandbox unavailable in example environment (%s); using sandbox_mode=disabled", | ||||||||||||||||
| cap.restriction_model, | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| extensions: list[ExampleExtensionBase] = [] | ||||||||||||||||
| extension_dir = os.path.join(base_path, "extensions") | ||||||||||||||||
| for extension in os.listdir(extension_dir): | ||||||||||||||||
|
|
@@ -85,6 +97,7 @@ class CustomConfig(TypedDict): | |||||||||||||||
| dependencies=manifest["dependencies"] + pyisolate_install, | ||||||||||||||||
| apis=[DatabaseSingleton], | ||||||||||||||||
| share_torch=manifest["share_torch"], | ||||||||||||||||
| sandbox_mode=sandbox_mode, | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| extension = manager.load_extension(config) | ||||||||||||||||
|
|
@@ -118,12 +131,7 @@ class CustomConfig(TypedDict): | |||||||||||||||
|
|
||||||||||||||||
| # Test Extension 2 | ||||||||||||||||
| ext2_result = await db.get_value("extension2_result") | ||||||||||||||||
| if ( | ||||||||||||||||
| ext2_result | ||||||||||||||||
| and ext2_result.get("extension") == "extension2" | ||||||||||||||||
| and ext2_result.get("array_sum") == 17.5 | ||||||||||||||||
| and ext2_result.get("numpy_version").startswith("2.") | ||||||||||||||||
| ): | ||||||||||||||||
| if ext2_result and ext2_result.get("extension") == "extension2" and ext2_result.get("array_sum") == 17.5: | ||||||||||||||||
|
||||||||||||||||
| if ext2_result and ext2_result.get("extension") == "extension2" and ext2_result.get("array_sum") == 17.5: | |
| if ( | |
| ext2_result | |
| and ext2_result.get("extension") == "extension2" | |
| and ext2_result.get("array_sum") == 17.5 | |
| and ext2_result.get("numpy_version").startswith("2.") | |
| ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
inspect.isawaitable check suggests API uncertainty — example should demonstrate the canonical call pattern.
If extension.stop() is synchronous, just call it. If it's async, await it. The isawaitable guard obscures the intended API usage for readers of the example. Examples are "tested in CI" and should "demonstrate real use cases," per guidelines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example/host.py` around lines 180 - 182, The example currently uses
inspect.isawaitable around extension.stop(), which hides the intended API;
change the example to use the canonical async pattern by calling await
extension.stop() directly (and update any example extension implementations to
make stop an async def) so the example consistently demonstrates the async API
for extension.stop(); alternatively, if the intended API is synchronous, remove
the await and call extension.stop() directly—pick one canonical contract and
make extension.stop() implementations and the example match it.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,9 +173,20 @@ def exclude_satisfied_requirements( | |
| """ | ||
| from packaging.requirements import Requirement | ||
|
|
||
| result = subprocess.run( # noqa: S603 # Trusted: system pip executable | ||
| [str(python_exe), "-m", "pip", "list", "--format", "json"], capture_output=True, text=True, check=True | ||
| ) | ||
| try: | ||
| result = subprocess.run( # noqa: S603 # Trusted: system pip executable | ||
| [str(python_exe), "-m", "pip", "list", "--format", "json"], | ||
| capture_output=True, | ||
| text=True, | ||
| check=True, | ||
| ) | ||
| except subprocess.CalledProcessError as exc: | ||
| # Newer uv versions can create venvs without pip unless seeded. | ||
| # If pip is unavailable, skip filtering and install requested deps. | ||
| if "No module named pip" in (exc.stderr or ""): | ||
| logger.debug("pip unavailable in %s; skipping satisfied-requirement filter", python_exe) | ||
| return requirements | ||
| raise | ||
| installed = {pkg["name"].lower(): pkg["version"] for pkg in json.loads(result.stdout)} | ||
| torch_ecosystem = get_torch_ecosystem_packages() | ||
|
|
||
|
|
@@ -227,6 +238,7 @@ def create_venv(venv_path: Path, config: ExtensionConfig) -> None: | |
| uv_path, | ||
| "venv", | ||
| str(venv_path), | ||
| "--seed", | ||
| "--python", | ||
| sys.executable, | ||
| ] | ||
|
|
@@ -337,7 +349,34 @@ def install_dependencies(venv_path: Path, config: ExtensionConfig, name: str) -> | |
| except Exception as exc: | ||
| logger.debug("Dependency cache read failed: %s", exc) | ||
|
|
||
| cmd = cmd_prefix + safe_deps + common_args | ||
| install_targets: list[str] = [] | ||
| i = 0 | ||
| while i < len(safe_deps): | ||
| dep = safe_deps[i] | ||
| dep_stripped = dep.strip() | ||
|
|
||
| # Support split editable args from existing callers: | ||
| # ["-e", "/path/to/pkg"]. | ||
| if dep_stripped == "-e": | ||
| if i + 1 >= len(safe_deps): | ||
| raise ValueError("Editable dependency '-e' must include a path or URL") | ||
| editable_target = safe_deps[i + 1].strip() | ||
| if not editable_target: | ||
| raise ValueError("Editable dependency '-e' must include a path or URL") | ||
| install_targets.extend(["-e", editable_target]) | ||
| i += 2 | ||
| continue | ||
|
|
||
| if dep_stripped.startswith("-e "): | ||
| editable_target = dep_stripped[3:].strip() | ||
| if not editable_target: | ||
| raise ValueError("Editable dependency must include a path or URL after '-e'") | ||
| install_targets.extend(["-e", editable_target]) | ||
| else: | ||
|
Comment on lines
+360
to
+375
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reject option-like editable targets in split/combined A target that still starts with 🛠️ Suggested validation hardening if dep_stripped == "-e":
if i + 1 >= len(safe_deps):
raise ValueError("Editable dependency '-e' must include a path or URL")
editable_target = safe_deps[i + 1].strip()
if not editable_target:
raise ValueError("Editable dependency '-e' must include a path or URL")
+ if editable_target.startswith("-"):
+ raise ValueError(
+ "Editable dependency target after '-e' cannot start with '-'"
+ )
install_targets.extend(["-e", editable_target])
i += 2
continue
if dep_stripped.startswith("-e "):
editable_target = dep_stripped[3:].strip()
if not editable_target:
raise ValueError("Editable dependency must include a path or URL after '-e'")
+ if editable_target.startswith("-"):
+ raise ValueError(
+ "Editable dependency target after '-e' cannot start with '-'"
+ )
install_targets.extend(["-e", editable_target])🤖 Prompt for AI Agents |
||
| install_targets.append(dep) | ||
| i += 1 | ||
|
|
||
| cmd = cmd_prefix + install_targets + common_args | ||
|
|
||
| with subprocess.Popen( # noqa: S603 # Trusted: validated pip/uv install cmd | ||
| cmd, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyTorch 2.3.0 is no longer tested on Windows.
Reducing the matrix to
['2.1.0']only means Windows-specific regressions for PyTorch 2.3.x (the current release line at the time of this PR) go undetected. If CI time is a concern, consider keeping 2.3.0 and dropping 2.1.0 instead, since 2.1.x is now EOL.🤖 Prompt for AI Agents