Cherry-pick run.py Docker diagnostics from PR #607#609
Conversation
- Added `verify_docker_containers` function to check the status of Docker containers and return their health information.
…on in run.py - Introduced `run_command_with_error_reporting` to enhance command execution with detailed error messages and suggestions for common issues. - Added `report_file_operation_error` and `safe_file_operation` for improved file operation error handling. - Implemented `suggest_docker_fixes` and `suggest_pip_fixes` to provide users with actionable solutions for Docker and pip-related errors. - Enhanced `parse_docker_build_failure` to identify failing containers and extract relevant error information. - Updated `show_docker_build_progress_header` to display building status for Docker containers.
Add comprehensive test suite for Docker build failure detection. Includes automated venv setup and 'python run.py tests build' command.
- Improved `parse_docker_build_failure` to provide more detailed insights into build errors. - Updated error reporting functions to include suggestions for resolving Docker-related issues. - Refined the overall error handling mechanism to enhance user experience during Docker operations.
There was a problem hiding this comment.
Pull request overview
This PR expands run.py to provide richer Docker compose build/start diagnostics (including container-status verification, guided troubleshooting output, and a new tests build command), while also introducing a pytest-based Docker build test suite and simple CLI install/uninstall wrappers.
Changes:
- Add Docker compose output streaming + build-failure parsing, container startup verification, and richer health-check error reporting in
run.py. - Introduce
python run.py tests buildwith automatic venv setup and a new pytest suite to validate Docker build failure detection. - Add
tt-studiowrapper plusinstall.sh/uninstall.shfor a symlink-based CLI install.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
run.py |
Adds compose progress streaming, build/start diagnostics, container verification, and a new tests build command path. |
tests/test_docker_builds.py |
Adds Docker build success/failure tests (with injected Dockerfile failures) + compose config validation for Chroma. |
tests/conftest.py |
Adds shared pytest fixtures and helpers for docker build/compose build execution. |
tests/README.md |
Documents how to run the new tests (but currently has broken code fences). |
tests/__init__.py |
Initializes the new tests package. |
pytest.ini |
Adds pytest discovery + markers + strict marker enforcement and timeout config. |
dev-tools/requirements-dev.txt |
Adds pytest tooling deps needed for the new test suite. |
tt-studio |
Adds a wrapper script to invoke run.py via python3. |
install.sh |
Adds symlink-based install to ~/.local/bin/tt-studio. |
uninstall.sh |
Adds removal of the installed symlink. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
There was a problem hiding this comment.
run_command_with_error_reporting, report_file_operation_error, and safe_file_operation are added but never used anywhere in this script. This increases maintenance surface and can confuse future edits; either remove them or refactor call sites to use them consistently (e.g., replace ad-hoc subprocess/file error handling).
| # Keep references to helper functions to avoid them being treated as unused. | |
| _HELPER_FUNCS = ( | |
| run_command_with_error_reporting, | |
| report_file_operation_error, | |
| safe_file_operation, | |
| ) |
| try: | ||
| # Run docker ps to check running containers | ||
| result = subprocess.run( | ||
| ["docker", "ps", "-a", "--filter", "name=tt_studio", "--format", "{{.Names}}\t{{.Status}}"], | ||
| capture_output=True, | ||
| text=True, | ||
| check=False | ||
| ) |
There was a problem hiding this comment.
verify_docker_containers() always runs docker ps without sudo. If the user lacks docker socket permissions (the code path where you run docker compose via sudo), this verification will fail and incorrectly abort startup. Consider using run_docker_command(..., use_sudo=not has_docker_access) or add a use_sudo/has_docker_access parameter.
| try: | ||
| print(f"{C_BLUE}🛑 Stopping Docker containers...{C_RESET}") | ||
| result = run_docker_command(docker_compose_cmd, use_sudo=not has_docker_access, capture_output=False) | ||
| result = run_docker_command(docker_compose_cmd, use_sudo=not has_docker_access, capture_output=True) | ||
| if result.returncode == 0: | ||
| print(f"{C_GREEN}✅ Docker containers stopped successfully.{C_RESET}") | ||
| else: | ||
| print(f"{C_GREEN}✅ Docker containers stopped successfully.{C_RESET}") | ||
| print(f"{C_YELLOW}⚠️ Warning: Docker compose down reported issues{C_RESET}") | ||
| print(f"{C_YELLOW} Exit code: {result.returncode}{C_RESET}") | ||
| if hasattr(result, 'stderr') and result.stderr: | ||
| print(f"{C_YELLOW} {result.stderr}{C_RESET}") |
There was a problem hiding this comment.
cleanup_resources() now passes capture_output=True to run_docker_command, but run_docker_command ignores capture_output when use_sudo=True (it always runs sudo with capture_output=False). As a result, the new stderr-based warnings often won’t show details in the common “needs sudo” case; consider teaching run_docker_command to capture output for sudo runs (while still allowing interactive prompts when needed).
| venv_python = os.path.join(venv_dir, "bin", "python3") | ||
| venv_pip = os.path.join(venv_dir, "bin", "pip") |
There was a problem hiding this comment.
run_build_tests() hardcodes the venv interpreter to .venv/bin/python3. Some virtualenvs only guarantee .venv/bin/python (and on non-POSIX platforms the path is different). Prefer deriving the interpreter path more robustly (e.g., .venv/bin/python on POSIX, or use sys.executable after activation logic).
| venv_python = os.path.join(venv_dir, "bin", "python3") | |
| venv_pip = os.path.join(venv_dir, "bin", "pip") | |
| if os.name == "nt": | |
| venv_bin_dir = os.path.join(venv_dir, "Scripts") | |
| else: | |
| venv_bin_dir = os.path.join(venv_dir, "bin") | |
| venv_python = os.path.join(venv_bin_dir, "python") | |
| venv_pip = os.path.join(venv_bin_dir, "pip") |
| ### Advanced Method (Direct pytest) | ||
|
|
||
| ```bash | ||
| # Run all tests | ||
| pytest | ||
|
|
||
| ### Run specific test files | ||
|
|
||
| ```bash | ||
| pytest tests/test_docker_builds.py | ||
| ``` |
There was a problem hiding this comment.
The Markdown code fences are unbalanced: a bash block starts under “Advanced Method” but isn’t closed before the next headings, and another bash starts inside it. This breaks rendering; close the first code fence after pytest and remove the nested fence.
| print(f"\n # View all container logs:") | ||
| print(f" docker logs -f tt_studio_backend") | ||
| print(f" docker logs -f tt_studio_frontend") | ||
| print(f" docker logs -f tt_studio_chroma") | ||
|
|
There was a problem hiding this comment.
The diagnostic commands omit the agent container and also always suggest docker logs ... without sudo. Add tt_studio_agent to the “all container logs” list, and consider printing sudo-prefixed commands (or conditional guidance) when the user doesn’t have docker socket access.
| def run_build_tests(): | ||
| """Run Docker build tests with automatic virtual environment setup.""" | ||
| print(f"\n{C_TT_PURPLE}{C_BOLD}🧪 Docker Build Tests{C_RESET}") | ||
| print(f"{C_YELLOW}{'=' * 60}{C_RESET}\n") | ||
|
|
There was a problem hiding this comment.
The PR description/title indicate a cherry-pick of Docker diagnostics from PR #607, but this PR also adds install/uninstall scripts and a full pytest suite/config. Please update the PR description/title to match the actual scope, or split unrelated changes into separate PRs for easier review and rollback.
| print(f"📦 Installing Python requirements from requirements-api.txt...") | ||
| try: | ||
| run_command([venv_pip, "install", "-r", "requirements-api.txt"], check=True) | ||
| result = subprocess.run( |
There was a problem hiding this comment.
Variable result is not used.
| result = subprocess.run( | |
| subprocess.run( |
| """ | ||
|
|
||
| import shutil | ||
| from pathlib import Path |
There was a problem hiding this comment.
Import of 'Path' is not used.
| from pathlib import Path |
| } | ||
|
|
||
| # Look for error patterns like "[tt_studio_backend 7/8] RUN ..." | ||
| import re |
There was a problem hiding this comment.
This import of module re is redundant, as it was previously imported on line 35.
| import re |
Cherry-picks commit
6f3b34afrom PR #607 to enable independent merging of Docker container verification improvements.Changes
verify_docker_containers(): Queriesdocker psto collect container status, returns structured health dataprint_container_diagnostics(): Surfaces actionable troubleshooting commands when containers fail to startwait_for_all_services(): Addsskip_docker_controlparameterContext
Original commit by @anirudTT adds runtime container health checks and diagnostic output to catch Docker startup failures earlier with better error messages.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.