Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 72 additions & 38 deletions agentstack/packaging.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import os, sys
from typing import Optional, Callable
import os
import sys
from typing import Callable
from pathlib import Path
import re
import subprocess
import select
import site
from packaging.requirements import Requirement
from agentstack import conf, log
Expand All @@ -20,18 +20,19 @@
# the packages are installed into the correct virtual environment.
# In testing, when this was not set, packages could end up in the pyenv's
# site-packages directory; it's possible an environment variable can control this.
def _get_executeable_paths():
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had just done this when the _python_executable PR merged in. I'm not sure if my version is worse than the other one, but since both paths mattered based on the OS I smashed them into one func at the top of the file.

"""Get environment paths based on platform."""
python_path = '.venv/Scripts/python.exe' if sys.platform == 'win32' else '.venv/bin/python'
venv_path = conf.PATH / VENV_DIR_NAME.absolute()
return venv_path, python_path

_python_executable = ".venv/bin/python"

def set_python_executable(path: str):
global _python_executable

_python_executable = path
_VENV_PATH, _PYTHON_EXECUTABLE = _get_executeable_paths()


def install(package: str):
"""Install a package with `uv` and add it to pyproject.toml."""
global _python_executable
global _PYTHON_EXECUTABLE
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance this constant will change? The only reason to use global is if the function will mutate it causing it to function scope.

from agentstack.cli.spinner import Spinner

def on_progress(line: str):
Expand All @@ -40,43 +41,49 @@ def on_progress(line: str):

def on_error(line: str):
log.error(f"uv: [error]\n {line.strip()}")

with Spinner(f"Installing {package}") as spinner:
_wrap_command_with_callbacks(
[get_uv_bin(), 'add', '--python', _python_executable, package],
[get_uv_bin(), 'add', '--python', _PYTHON_EXECUTABLE, package],
on_progress=on_progress,
on_error=on_error,
)


def install_project():
"""Install all dependencies for the user's project."""
global _python_executable
global _PYTHON_EXECUTABLE
from agentstack.cli.spinner import Spinner

def on_progress(line: str):
if RE_UV_PROGRESS.match(line):
spinner.clear_and_log(line.strip(), 'info')

def on_error(line: str):
log.error(f"uv: [error]\n {line.strip()}")
log.error(f"UV installation error:\n{line.strip()}")

try:
with Spinner(f"Installing project dependencies.") as spinner:
with Spinner("Installing project dependencies...") as spinner:
result = _wrap_command_with_callbacks(
[get_uv_bin(), 'pip', 'install', '--python', _python_executable, '.'],
[get_uv_bin(), 'pip', 'install', '--python', _PYTHON_EXECUTABLE, '.'],
on_progress=on_progress,
on_error=on_error,
)
if result is False:
spinner.clear_and_log("Retrying uv installation with --no-cache flag...", 'info')
_wrap_command_with_callbacks(
[get_uv_bin(), 'pip', 'install', '--no-cache', '--python', _python_executable, '.'],
spinner.clear_and_log(
"⚠️ Initial installation failed, retrying with --no-cache flag...", 'warning'
)
result = _wrap_command_with_callbacks(
[get_uv_bin(), 'pip', 'install', '--no-cache', '--python', _PYTHON_EXECUTABLE, '.'],
on_progress=on_progress,
on_error=on_error,
)
if result is False:
raise Exception("Installation failed with --no-cache")
else:
spinner.clear_and_log("✨ All dependencies installed successfully!", 'success')
except Exception as e:
log.error(f"Installation failed: {str(e)}")
log.error(f"❌ Installation failed: {str(e)}")
raise


Expand All @@ -95,7 +102,7 @@ def on_error(line: str):

log.info(f"Uninstalling {requirement.name}")
_wrap_command_with_callbacks(
[get_uv_bin(), 'remove', '--python', _python_executable, requirement.name],
[get_uv_bin(), 'remove', '--python', _PYTHON_EXECUTABLE, requirement.name],
on_progress=on_progress,
on_error=on_error,
)
Expand All @@ -119,7 +126,7 @@ def on_error(line: str):

log.info(f"Upgrading {package}")
_wrap_command_with_callbacks(
[get_uv_bin(), 'pip', 'install', '-U', '--python', _python_executable, *extra_args, package],
[get_uv_bin(), 'pip', 'install', '-U', '--python', _PYTHON_EXECUTABLE, *extra_args, package],
on_progress=on_progress,
on_error=on_error,
use_venv=use_venv,
Expand All @@ -128,7 +135,7 @@ def on_error(line: str):

def create_venv(python_version: str = DEFAULT_PYTHON_VERSION):
"""Initialize a virtual environment in the project directory of one does not exist."""
if os.path.exists(conf.PATH / VENV_DIR_NAME):
if os.path.exists(_VENV_PATH):
return # venv already exists

RE_VENV_PROGRESS = re.compile(r'^(Using|Creating)')
Expand Down Expand Up @@ -160,7 +167,7 @@ def get_uv_bin() -> str:
def _setup_env() -> dict[str, str]:
"""Copy the current environment and add the virtual environment path for use by a subprocess."""
env = os.environ.copy()
env["VIRTUAL_ENV"] = str(conf.PATH / VENV_DIR_NAME.absolute())
env["VIRTUAL_ENV"] = str(_VENV_PATH)
env["UV_INTERNAL__PARENT_INTERPRETER"] = sys.executable
return env

Expand All @@ -184,33 +191,60 @@ def _wrap_command_with_callbacks(
}
if use_venv:
sub_args['env'] = _setup_env()

log.debug(f"Running command: {' '.join(command)}")
process = subprocess.Popen(command, **sub_args) # type: ignore
assert process.stdout and process.stderr # appease type checker

readable = [process.stdout, process.stderr]
while readable:
ready, _, _ = select.select(readable, [], [])
for fd in ready:
line = fd.readline()
if not line:
readable.remove(fd)
try:
while process.poll() is None:
try:
stdout, stderr = process.communicate(timeout=1.0)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

process.communicate is the solution to the main issue on this PR.

Using it required changing around the std in/out handling a touch, but the internet tells me .communicate is more cross-platform compatible. I'll be testing on my mac when I find the password -.-

if stdout:
on_progress(stdout)
all_lines += stdout
if stderr:
on_progress(stderr)
all_lines += stderr
except subprocess.TimeoutExpired:
continue

on_progress(line)
all_lines += line

if process.wait() == 0: # return code: success
except Exception as e:
log.error(f"Error reading output: {e}")
break

# Get any remaining output
stdout, stderr = process.communicate()
if stdout:
on_progress(stdout)
all_lines += stdout
if stderr:
on_progress(stderr)
all_lines += stderr

except Exception as e:
log.error(f"Error during output reading: {e}")
process.kill()
raise

return_code = process.wait()
log.debug(f"Process completed with return code: {return_code}")

if return_code == 0:
on_complete(all_lines)
return True
else:
error_msg = f"Process failed with return code {return_code}"
log.error(error_msg)
on_error(all_lines)
return False
except Exception as e:
on_error(str(e))
error_msg = f"Exception running command: {str(e)}"
log.error(error_msg)
on_error(error_msg)
return False
finally:
if process:
try:
process.terminate()
except:
pass
except Exception as e:
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ruff barked at me for an naked except so I tossed this in here. If my linter is the problem I'd love to know how to make ruff obey the pyproject.toml. It also happens if I run the pre-commit like I usually do.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the right move is to have it catch something that is of type Exception which is everything. If you any to go into best practice mode, it would be catch specific exceptions, but a catch all is fine if it doesn't get hidden and out the app in a weird state.

log.error(f"Error terminating process: {e}")
24 changes: 16 additions & 8 deletions tests/test_cli_init.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import os, sys
import os
import unittest
from parameterized import parameterized
from pathlib import Path
import shutil
from cli_test_utils import run_cli
from agentstack import conf
from agentstack import frameworks
from agentstack.cli import init_project
from agentstack.templates import get_all_templates

BASE_PATH = Path(__file__).parent
Expand All @@ -15,10 +14,16 @@
class CLIInitTest(unittest.TestCase):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use functional pytest patterns and fixtures within conftests etc instead of using unittest.TestCase. It's the more supported pattern now a days and much easier to expand with reusable fixtures etc as the test suite grows.

def setUp(self):
self.framework = os.getenv('TEST_FRAMEWORK')
self.project_dir = Path(BASE_PATH / 'tmp' / self.framework / 'test_cli_init')
os.chdir(BASE_PATH) # Change to parent directory first
self.project_dir = BASE_PATH / 'tmp' / self.framework / 'test_repo'
os.chdir(str(BASE_PATH)) # Change directory before cleanup to avoid Windows file locks

# Clean up any existing test directory
if self.project_dir.exists():
shutil.rmtree(self.project_dir, ignore_errors=True)

os.makedirs(self.project_dir, exist_ok=True)
os.chdir(self.project_dir)
os.chdir(self.project_dir) # gitpython needs a cwd

# Force UTF-8 encoding for the test environment
os.environ['PYTHONIOENCODING'] = 'utf-8'

Expand All @@ -38,7 +43,10 @@ def test_init_command_aliased_framework_empty_project(self, alias: str, framewor
if framework != self.framework:
self.skipTest(f"{alias} is not related to this framework")

conf.set_path(self.project_dir) # set working dir, init adds `slug_name`
init_project(slug_name='test_project', template='empty', framework=alias)
result = run_cli('init', 'test_project', '--template', 'empty', '--framework', alias)
self.assertEqual(result.returncode, 0)

# Verify the framework was set correctly
conf.set_path(self.project_dir / 'test_project')
config = conf.ConfigFile()
assert config.framework == framework
self.assertEqual(config.framework, framework)
23 changes: 16 additions & 7 deletions tests/test_cli_tools.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import subprocess
import os, sys
import os
import unittest
from parameterized import parameterized
from pathlib import Path
Expand All @@ -15,12 +14,22 @@
BASE_PATH = Path(__file__).parent
TEMPLATE_NAME = "empty"


class CLIToolsTest(unittest.TestCase):
def setUp(self):
self.framework = os.getenv('TEST_FRAMEWORK')
self.project_dir = BASE_PATH / 'tmp' / self.framework / 'cli_tools'
self.project_dir = BASE_PATH / 'tmp' / self.framework / 'test_repo'
os.chdir(str(BASE_PATH)) # Change directory before cleanup to avoid Windows file locks

# Clean up any existing test directory
if self.project_dir.exists():
shutil.rmtree(self.project_dir, ignore_errors=True)

os.makedirs(self.project_dir, exist_ok=True)
os.chdir(self.project_dir)
os.chdir(self.project_dir) # gitpython needs a cwd

# Force UTF-8 encoding for the test environment
os.environ['PYTHONIOENCODING'] = 'utf-8'

def tearDown(self):
shutil.rmtree(self.project_dir, ignore_errors=True)
Expand Down Expand Up @@ -133,8 +142,8 @@ def test_create_tool_invalid_name(self):
# Create agent
run_cli('generate', 'agent', 'test_agent', '--llm', 'openai/gpt-4')

# Test various invalid names
invalid_names = ['TestTool', 'test-tool', 'test tool']
# Quote the space-containing name (for win)
invalid_names = ['TestTool', 'test-tool', '"test tool"']
for name in invalid_names:
result = run_cli('tools', 'new', name)
self.assertNotEqual(result.returncode, 0)
Expand All @@ -144,4 +153,4 @@ def test_create_tool_no_project(self):
"""Test creating a tool outside a project directory"""
# Try to create tool without initializing project
result = run_cli('tools', 'new', 'test_tool')
self.assertNotEqual(result.returncode, 0)
self.assertNotEqual(result.returncode, 0)
Loading