Skip to content

Commit

Permalink
remove use of aiofiles (#1024)
Browse files Browse the repository at this point in the history
* remove use of aiofiles

We currently use aiofiles for sandbox read_file implementations as well as for writing auto-compose files. These are all local file reads and they are for small files so in most cases this will make read performance considerably worse (as it schedules the read/write onto a thread pool).

Further, we are attempting to eliminate any possibility of threading deadlocks, and this represents another point of exposure to them.

* update changelog

---------

Co-authored-by: J.J. Allaire <[email protected]>
  • Loading branch information
jjallaire-aisi and jjallaire authored Dec 19, 2024
1 parent b8bfb9b commit ac922a4
Show file tree
Hide file tree
Showing 7 changed files with 24 additions and 29 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
- Bedrock: Correct max_tokens for llama3-8b, llama3-70b models on Bedrock.
- Inspect View: Various improvements to appearance of tool calls in transcript.
- Task display: Ensure that widths of progress elements are kept consistant across tasks.
- Sandboxes: Remove use of aiofiles to mitigate potential for threading deadlocks.
- Bugfix: Proper handling of text find for eval raw JSON display
- Bugfix: Correct handling for `--sample-id` integer comparisons.
- Bugfix: Proper removal of model_args with falsey values (explicit check for `None`)
Expand Down
1 change: 0 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ dev = [
"ruff==0.8.3", # match version specified in .pre-commit-config.yaml
"textual-dev>=0.86.2",
"types-PyYAML",
"types-aiofiles",
"types-beautifulsoup4",
"types-aioboto3",
"types-boto3",
Expand Down
1 change: 0 additions & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
aiofiles
aiohttp>=3.9.0
anyio>=4.4.0 # not directly required, pinned by Snyk to avoid a vulnerability
beautifulsoup4
Expand Down
18 changes: 8 additions & 10 deletions src/inspect_ai/util/_sandbox/docker/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
from logging import getLogger
from pathlib import Path

import aiofiles

logger = getLogger(__name__)


Expand All @@ -17,7 +15,7 @@
DOCKERFILE = "Dockerfile"


async def resolve_compose_file(parent: str = "") -> str:
def resolve_compose_file(parent: str = "") -> str:
# existing compose file provides all the config we need
compose = find_compose_file(parent)
if compose is not None:
Expand All @@ -29,11 +27,11 @@ async def resolve_compose_file(parent: str = "") -> str:

# dockerfile just needs a compose.yaml synthesized
elif has_dockerfile(parent):
return await auto_compose_file(COMPOSE_DOCKERFILE_YAML, parent)
return auto_compose_file(COMPOSE_DOCKERFILE_YAML, parent)

# otherwise provide a generic python container
else:
return await auto_compose_file(COMPOSE_GENERIC_YAML, parent)
return auto_compose_file(COMPOSE_GENERIC_YAML, parent)


def find_compose_file(parent: str = "") -> str | None:
Expand All @@ -59,9 +57,9 @@ def is_auto_compose_file(file: str) -> bool:
return os.path.basename(file) == AUTO_COMPOSE_YAML


async def ensure_auto_compose_file(file: str | None) -> None:
def ensure_auto_compose_file(file: str | None) -> None:
if file is not None and is_auto_compose_file(file) and not os.path.exists(file):
await resolve_compose_file(os.path.dirname(file))
resolve_compose_file(os.path.dirname(file))


def safe_cleanup_auto_compose(file: str | None) -> None:
Expand Down Expand Up @@ -100,8 +98,8 @@ def safe_cleanup_auto_compose(file: str | None) -> None:
"""


async def auto_compose_file(contents: str, parent: str = "") -> str:
def auto_compose_file(contents: str, parent: str = "") -> str:
path = os.path.join(parent, AUTO_COMPOSE_YAML)
async with aiofiles.open(path, "w", encoding="utf-8") as f:
await f.write(contents)
with open(path, "w", encoding="utf-8") as f:
f.write(contents)
return Path(path).resolve().as_posix()
9 changes: 4 additions & 5 deletions src/inspect_ai/util/_sandbox/docker/docker.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
from pathlib import Path, PurePosixPath
from typing import Literal, Union, cast, overload

import aiofiles
from typing_extensions import override

from inspect_ai.util._subprocess import ExecResult
Expand Down Expand Up @@ -403,11 +402,11 @@ async def read_file(self, file: str, text: bool = True) -> Union[str, bytes]:

# read and return w/ appropriate encoding
if text:
async with aiofiles.open(dest_file, "r", encoding="utf-8") as f:
return await f.read()
with open(dest_file, "r", encoding="utf-8") as f:
return f.read()
else:
async with aiofiles.open(dest_file, "rb") as f:
return await f.read()
with open(dest_file, "rb") as f:
return f.read()

@override
async def connection(self) -> SandboxConnection:
Expand Down
6 changes: 3 additions & 3 deletions src/inspect_ai/util/_sandbox/docker/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ async def create(

# if its a Dockerfile, then config is the auto-generated .compose.yaml
if config_path and is_dockerfile(config_path.name):
config = await auto_compose_file(
config = auto_compose_file(
COMPOSE_DOCKERFILE_YAML, config_path.parent.as_posix()
)

Expand All @@ -51,12 +51,12 @@ async def create(

# no config passed, look for 'auto-config' (compose.yaml, Dockerfile, etc.)
else:
config = await resolve_compose_file()
config = resolve_compose_file()

# this could be a cleanup where docker has tracked a .compose.yaml file
# as part of its ConfigFiles and passed it back to us -- we in the
# meantime have cleaned it up so we re-create it here as required
await ensure_auto_compose_file(config)
ensure_auto_compose_file(config)

# return project
return ComposeProject(name, config, env)
Expand Down
17 changes: 8 additions & 9 deletions src/inspect_ai/util/_sandbox/local.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
from pathlib import Path
from typing import Literal, Union, cast, overload

import aiofiles
from typing_extensions import override

from .._subprocess import ExecResult, subprocess
Expand Down Expand Up @@ -85,11 +84,11 @@ async def write_file(self, file: str, contents: str | bytes) -> None:
Path(file).parent.mkdir(parents=True, exist_ok=True)

if isinstance(contents, str):
async with aiofiles.open(file, "w", encoding="utf-8") as f:
await f.write(contents)
with open(file, "w", encoding="utf-8") as f:
f.write(contents)
else:
async with aiofiles.open(file, "wb") as f:
await f.write(contents)
with open(file, "wb") as f:
f.write(contents)

@overload
async def read_file(self, file: str, text: Literal[True] = True) -> str: ...
Expand All @@ -102,11 +101,11 @@ async def read_file(self, file: str, text: bool = True) -> Union[str | bytes]:
file = self._resolve_file(file)
verify_read_file_size(file)
if text:
async with aiofiles.open(file, "r", encoding="utf-8") as f:
return await f.read()
with open(file, "r", encoding="utf-8") as f:
return f.read()
else:
async with aiofiles.open(file, "rb") as f:
return await f.read()
with open(file, "rb") as f:
return f.read()

def _resolve_file(self, file: str) -> str:
path = Path(file)
Expand Down

0 comments on commit ac922a4

Please sign in to comment.