-
-
Notifications
You must be signed in to change notification settings - Fork 385
Pyapp executable #154
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
base: main
Are you sure you want to change the base?
Pyapp executable #154
Conversation
WalkthroughAdds PyApp packaging and cross-compilation tooling, updates packaging/deps and .gitignore, makes resource resolution PYAPP-aware, introduces an automated Ollama installer and PYAPP-gated CLI auto-setup flow, provides a package entrypoint, and adds multiple default configs for TTS, app, and ASR. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as glados.cli
participant Auto as utils.automated_install
participant Res as utils.resources
participant TUI as glados.tui
User->>CLI: run `python -m glados` (PYAPP=1)
Note over CLI: non-download command path
CLI->>Auto: automated_install()
Auto->>Auto: check/install Ollama (per-OS)
Auto->>Res: resolve default config paths (PYAPP-aware)
Auto->>Auto: ensure defaults exist / validate or download models / ollama pull
Auto-->>CLI: return
CLI->>TUI: tui(DEFAULT_CONFIG)
TUI->>Res: load assets via resource_path()
TUI-->>User: UI starts
sequenceDiagram
autonumber
participant Env as Environment
participant Res as utils.resources
Env-->>Res: PYAPP != "1"
Res->>Res: get_package_root() -> climb to project root
Res-->>Env: resource_path = project_root / relative_path
Env-->>Res: PYAPP == "1"
Res->>Res: get_package_root() -> package dir (no climb)
Res-->>Env: resource_path = PYAPP_RELATIVE_DIR / relative_path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 8
🧹 Nitpick comments (17)
src/glados/default_configs/glados_config.yaml (1)
3-3: Use 127.0.0.1 to avoid potential HttpUrl validation issuesIf pydantic HttpUrl rejects “localhost” on some versions, prefer 127.0.0.1 for maximum compatibility.
- completion_url: "http://localhost:11434/api/chat" + completion_url: "http://127.0.0.1:11434/api/chat"scripts/compile_pyapp.sh (3)
21-26: Avoid hardcoding VERSION/WHEEL_PATH; read from pyproject.tomlKeeps artifacts in sync with releases.
-VERSION="0.1.0" -EXE_BASE="${BINARY_NAME}-${VERSION}" -WHEEL_PATH="$(pwd)/dist/${PROJECT_NAME}-${VERSION}-py3-none-any.whl" +VERSION="$(python - <<'PY' +import tomllib; print(tomllib.load(open("pyproject.toml","rb"))["project"]["version"]) +PY +)" +EXE_BASE="${BINARY_NAME}-${VERSION}" +WHEEL_PATH="$(pwd)/dist/${PROJECT_NAME}-${VERSION}-py3-none-any.whl"
37-39: Install cross only if missingAvoids reinstalling every run.
-cargo install cross --git https://github.com/cross-rs/cross +command -v cross >/dev/null 2>&1 || cargo install cross --git https://github.com/cross-rs/cross
156-161: AppRun relies on x-terminal-emulator; add fallbacks or run directlyImprove robustness across distros/DEs.
-cat <<'EOF' > "${APPDIR}/AppRun" +cat <<'EOF' > "${APPDIR}/AppRun" #!/bin/sh HERE="$(dirname "$(readlink -f "$0")")" -x-terminal-emulator -e "$HERE/usr/bin/GLaDOS" "$@" +EXEC="$HERE/usr/bin/GLaDOS" +if command -v x-terminal-emulator >/dev/null 2>&1; then + x-terminal-emulator -e "$EXEC" "$@" +elif command -v gnome-terminal >/dev/null 2>&1; then + gnome-terminal -- "$EXEC" "$@" +elif command -v konsole >/dev/null 2>&1; then + konsole -e "$EXEC" "$@" +else + exec "$EXEC" "$@" +fi EOF chmod +x "${APPDIR}/AppRun"src/glados/default_configs/parakeet-tdt-0.6b-v2_model_config.yaml (1)
49-54: Tokenzier paths point to an internal filesystemThese absolute paths won’t exist for users and are not needed for inference in your current pipeline. Remove or replace with neutral placeholders to avoid confusion.
-tokenizer: - dir: /lustre/fsw/portfolios/llmservice/users/nkoluguri/datasets/tokenizers/en_mm_ytc/tokenizer_spe_bpe_v1024/ - type: bpe - model_path: nemo:705f11d22dc04b169effc35ce5cd1361_tokenizer.model - vocab_path: nemo:4cf78c8ca4ca44fca36c3754478fb188_vocab.txt - spe_tokenizer_vocab: nemo:a4715c7f6b2d4c2bb709306073d0c0a4_tokenizer.vocab +# tokenizer: # Optional; not required for runtime decoding in this project +# type: bpesrc/glados/__main__.py (1)
3-4: Propagate CLI exit codeReturn non-zero codes to the shell.
-from .cli import main +from .cli import main +import sys if __name__ == "__main__": - main() + sys.exit(main())src/glados/tui.py (2)
20-20: Make CSS_PATH package-aware to avoid missing styles under PyApp/wheels.CSS is currently a relative path; it may fail when cwd ≠ project root. Use an absolute path from get_package_root().
Outside the changed lines, set:
# near class GladosUI definition from .utils.resources import get_package_root CSS_PATH = str(get_package_root() / "glados_ui/glados.tcss")
439-445: Fail fast if config is missing (avoid proceeding after logging error).Current flow logs an error but still attempts to load, leading to a secondary failure.
Apply this diff:
- config_path = resource_path("configs/glados_config.yaml") - if not config_path.exists(): - logger.error(f"GLaDOS config file not found: {config_path}") + config_path = resource_path("configs/glados_config.yaml") + if not config_path.exists(): + msg = f"GLaDOS config file not found: {config_path}" + logger.error(msg) + raise FileNotFoundError(msg)src/glados/utils/resources.py (1)
25-31: Guard missing PYAPP_RELATIVE_DIR and prefer cwd fallback.Avoid Path(None) when env isn’t set due to import order or manual runs.
Apply this diff:
- if os.getenv("PYAPP") == "1": - root_path = Path(os.getenv("PYAPP_RELATIVE_DIR")) # Relative path to the executable + if os.getenv("PYAPP") == "1": + rel = os.getenv("PYAPP_RELATIVE_DIR") + root_path = Path(rel) if rel else Path.cwd() # Relative to executable/cwd else: root_path = get_package_root()src/glados/cli.py (2)
24-24: Use str for DEFAULT_CONFIG to keep argparse help clean.Argparse prints PosixPath(...) in help when default is a Path.
Apply this diff:
-DEFAULT_CONFIG = resource_path("configs/glados_config.yaml") +DEFAULT_CONFIG = str(resource_path("configs/glados_config.yaml"))
291-295: Refine PyApp fast‑path: keep ‘say’ working; handle setup failures; drop unused arg.
- Don’t override ‘say’ when packaged.
- Show a friendly error if automated setup fails.
- tui() ignores its config_path argument—no need to pass DEFAULT_CONFIG.
Apply this diff:
- if os.getenv("PYAPP") == "1": - automated_install() - tui(DEFAULT_CONFIG) - return 0 + if os.getenv("PYAPP") == "1" and args.command in (None, "start", "tui"): + try: + automated_install() + except Exception as e: + print(f"Automated setup failed: {e}") + return 1 + tui() + return 0src/glados/utils/automated_install.py (6)
22-29: Windows: add timeouts and tool presence check for curl; chain exceptions.Prevents hangs and clearer error provenance.
Apply this diff:
+import shutil @@ - try: - subprocess.run(["curl", "-L", download_url, "-o", str(installer_path)], check=True) - subprocess.run([str(installer_path)], check=True) + try: + if shutil.which("curl") is None: + raise RuntimeError("curl not found in PATH; required to install Ollama on Windows") + subprocess.run(["curl", "-L", download_url, "-o", str(installer_path)], check=True, timeout=1200) + subprocess.run([str(installer_path)], check=True, timeout=1200) - except subprocess.CalledProcessError: - raise RuntimeError("Failed to install Ollama on Windows") + except subprocess.CalledProcessError as err: + raise RuntimeError("Failed to install Ollama on Windows") from err
40-57: macOS: remove unused variable; add tool checks/timeouts; chain exceptions.Also prefer shutil.which over invoking which.
Apply this diff:
- try: - # Check if Homebrew is installed - if subprocess.run(["which", "brew"], capture_output=True, text=True).returncode == 0: - subprocess.run(["brew", "install", "ollama"], check=True) + try: + if shutil.which("brew"): + subprocess.run(["brew", "install", "ollama"], check=True, timeout=1200) print("Ollama installed successfully on macOS via Homebrew") else: # Fallback to direct download download_url = "https://ollama.com/download/Ollama-darwin.zip" installer_path = Path("Ollama-darwin.zip") - app_path = Path("/Applications/Ollama.app") - - subprocess.run(["curl", "-L", download_url, "-o", str(installer_path)], check=True) - subprocess.run(["unzip", str(installer_path), "-d", "/Applications"], check=True) + if shutil.which("curl") is None: + raise RuntimeError("curl not found in PATH; required to install Ollama on macOS") + if shutil.which("unzip") is None: + raise RuntimeError("unzip not found in PATH; required to install Ollama on macOS") + subprocess.run(["curl", "-L", download_url, "-o", str(installer_path)], check=True, timeout=1200) + subprocess.run(["unzip", str(installer_path), "-d", "/Applications"], check=True, timeout=1200) print("Ollama installed successfully on macOS to /Applications") installer_path.unlink(missing_ok=True) - except subprocess.CalledProcessError: - raise RuntimeError("Failed to install Ollama on macOS") + except subprocess.CalledProcessError as err: + raise RuntimeError("Failed to install Ollama on macOS") from err
96-101: Guard PYAPP_RELATIVE_DIR; default to cwd for non‑PyApp/manual runs.Prevents crashes if the env isn’t set.
Apply this diff:
- for file in default_configuration_files: - relative_path = Path(os.getenv("PYAPP_RELATIVE_DIR")) / file + base_dir = Path(os.getenv("PYAPP_RELATIVE_DIR") or os.getcwd()) + for file in default_configuration_files: + relative_path = base_dir / file
110-113: Check download_models() result; bubble up failures.Currently ignores non‑zero return codes.
Apply this diff:
- if not models_valid(): + if not models_valid(): print("Some model files are invalid or missing, downloading...") - asyncio.run(download_models()) + rc = asyncio.run(download_models()) + if rc != 0: + raise RuntimeError("Model download failed; cannot continue")
114-121: YAML parsing: add safe fallback and clearer error on missing Glados.llm_model.Minor polish; keeps errors clean for end users.
Apply this diff:
- with (Path(os.getenv("PYAPP_RELATIVE_DIR")) / "configs/glados_config.yaml").open() as f: + with (Path(os.getenv("PYAPP_RELATIVE_DIR") or os.getcwd()) / "configs/glados_config.yaml").open() as f: config = yaml.safe_load(f) @@ - if not llm_model: - raise ValueError("No 'llm_model' specified in YAML file") + if not llm_model: + raise ValueError("Missing 'Glados.llm_model' in glados_config.yaml")
122-126: Add timeout and chain exception for ‘ollama pull’.Avoid indefinite hangs and retain original traceback.
Apply this diff:
- try: - subprocess.run(["ollama", "pull", llm_model], check=True) + try: + subprocess.run(["ollama", "pull", llm_model], check=True, timeout=1800) print(f"Model {llm_model} pulled successfully") - except subprocess.CalledProcessError: - raise RuntimeError(f"Failed to pull model: {llm_model}") + except subprocess.CalledProcessError as err: + raise RuntimeError(f"Failed to pull model: {llm_model}") from err
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
images/aperture_icon.icois excluded by!**/*.icoimages/aperture_icon.pngis excluded by!**/*.pngsrc/glados/default_configs/0.wavis excluded by!**/*.wavsrc/glados/default_configs/idx_to_token.pklis excluded by!**/*.pklsrc/glados/default_configs/lang_phoneme_dict.pklis excluded by!**/*.pklsrc/glados/default_configs/lang_phoneme_dict_old.pklis excluded by!**/*.pklsrc/glados/default_configs/phoneme_to_id.pklis excluded by!**/*.pklsrc/glados/default_configs/token_to_idx.pklis excluded by!**/*.pkl
📒 Files selected for processing (13)
.gitignore(1 hunks)README.md(1 hunks)pyproject.toml(2 hunks)scripts/compile_pyapp.sh(1 hunks)src/glados/__init__.py(1 hunks)src/glados/__main__.py(1 hunks)src/glados/cli.py(3 hunks)src/glados/default_configs/glados.json(1 hunks)src/glados/default_configs/glados_config.yaml(1 hunks)src/glados/default_configs/parakeet-tdt-0.6b-v2_model_config.yaml(1 hunks)src/glados/tui.py(4 hunks)src/glados/utils/automated_install.py(1 hunks)src/glados/utils/resources.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/glados/__main__.py (1)
src/glados/cli.py (1)
main(239-308)
src/glados/default_configs/glados_config.yaml (1)
src/glados/core/engine.py (3)
GladosConfig(62-119)Glados(122-377)PersonalityPrompt(33-59)
src/glados/default_configs/glados.json (1)
src/glados/TTS/tts_glados.py (3)
PiperConfig(20-97)__init__(124-168)from_dict(45-97)
src/glados/tui.py (1)
src/glados/utils/resources.py (2)
resource_path(23-30)get_package_root(9-20)
src/glados/utils/automated_install.py (2)
src/glados/utils/resources.py (1)
get_package_root(9-20)src/glados/cli.py (3)
main(239-308)download_models(115-146)models_valid(149-162)
src/glados/cli.py (1)
src/glados/utils/automated_install.py (1)
main(60-126)
src/glados/default_configs/parakeet-tdt-0.6b-v2_model_config.yaml (1)
src/glados/ASR/tdt_asr.py (1)
AudioTranscriber(251-558)
🪛 Ruff (0.12.2)
src/glados/utils/automated_install.py
12-12: Starting a process with a partial executable path
(S607)
13-13: Consider moving this statement to an else block
(TRY300)
23-23: subprocess call: check for execution of untrusted input
(S603)
23-23: Starting a process with a partial executable path
(S607)
24-24: subprocess call: check for execution of untrusted input
(S603)
27-27: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
27-27: Avoid specifying long messages outside the exception class
(TRY003)
34-34: subprocess call with shell=True seems safe, but may be changed in the future; consider rewriting without shell
(S602)
34-34: Starting a process with a partial executable path
(S607)
37-37: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
37-37: Avoid specifying long messages outside the exception class
(TRY003)
44-44: Starting a process with a partial executable path
(S607)
45-45: Starting a process with a partial executable path
(S607)
51-51: Local variable app_path is assigned to but never used
Remove assignment to unused variable app_path
(F841)
53-53: subprocess call: check for execution of untrusted input
(S603)
53-53: Starting a process with a partial executable path
(S607)
54-54: subprocess call: check for execution of untrusted input
(S603)
54-54: Starting a process with a partial executable path
(S607)
58-58: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
58-58: Avoid specifying long messages outside the exception class
(TRY003)
79-79: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
123-123: subprocess call: check for execution of untrusted input
(S603)
123-123: Starting a process with a partial executable path
(S607)
126-126: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
126-126: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (11)
.gitignore (1)
166-169: LGTM: ignore large model artifacts and PyApp workspaceIgnoring *.onnx and pyapp-latest/ will keep generated artifacts out of VCS.
scripts/compile_pyapp.sh (1)
41-54: Cross.toml volume mapping: verify cross-rs expects env var names onlyConfirm that volumes = ["PYAPP_PROJECT_PATH"] is the intended syntax for cross-rs to mount the host wheel path into the container.
Validation doc check suggested.
src/glados/default_configs/parakeet-tdt-0.6b-v2_model_config.yaml (1)
118-126: Resolved — labels length matches decoder.vocab_size (1024).
labels: 1024, decoder.vocab_size: 1024 — no action required.README.md (1)
199-208: Incorrect — no duplicate 'Build releases' heading foundSearch shows a single "## Build releases" at README.md:199. Ignore the original duplicate/deduplication request.
Likely an incorrect or invalid review comment.
src/glados/default_configs/glados.json (1)
1-18: Confirm config compatibility and ensure it’s packaged
- audio.sample_rate, espeak.voice, phoneme_id_map and num_symbols are present in src/glados/default_configs/glados.json.
- phoneme_id_map IDs span 0–156; num_symbols = 256 (>= 156+1) — in-range (no out-of-bounds IDs) but contains unused symbol slots; reduce num_symbols if you want a tight alphabet.
- Ensure src/glados/default_configs/glados.json is included in the wheel (pyproject.toml package_data/include) — packaging not verified here.
pyproject.toml (1)
19-20: Verify onnxruntime-gpu as a hard dependencyLocal verification failed: ModuleNotFoundError: No module named 'onnxruntime'. File: pyproject.toml (lines 19-20).
- Re-run verification in an environment that has the package installed (or CI image you target): python -c "import onnxruntime as ort; print(ort.get_available_providers())".
- If the GPU wheel is not guaranteed across target platforms, make onnxruntime-gpu optional (extras) or install it conditionally; document CPU fallback on Linux/Windows and macOS install behavior (pip may fail).
src/glados/tui.py (2)
172-176: Asset path resolution looks good.Loading splash.ansi via get_package_root() matches the new packaging strategy.
281-286: LGTM for logo asset loading.Consistent with splash handling and resilient with fallback.
src/glados/cli.py (2)
6-6: Import addition is fine.
17-18: Import alias for automated install looks good.src/glados/utils/automated_install.py (1)
39-41: Untested macOS path.Flagging for manual verification on a real macOS host.
Would you run the packaged app on macOS and confirm Homebrew and direct-download branches? Pay attention to permissions for writing into /Applications.
| [tool.hatch.build.targets.wheel] | ||
| packages = ["src/glados"] | ||
|
|
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.
Wheel misses non-Python resources (default configs) — include them
default_configs/.yaml|.json won’t be in the wheel by default, which will break resource loading in PyApp and pip installs. Include them in the wheel.
Apply:
[tool.hatch.build.targets.wheel]
-packages = ["src/glados"]
+packages = ["src/glados"]
+include = [
+ "src/glados/default_configs/**/*.yaml",
+ "src/glados/default_configs/**/*.yml",
+ "src/glados/default_configs/**/*.json",
+]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [tool.hatch.build.targets.wheel] | |
| packages = ["src/glados"] | |
| [tool.hatch.build.targets.wheel] | |
| packages = ["src/glados"] | |
| include = [ | |
| "src/glados/default_configs/**/*.yaml", | |
| "src/glados/default_configs/**/*.yml", | |
| "src/glados/default_configs/**/*.json", | |
| ] |
🤖 Prompt for AI Agents
In pyproject.toml around lines 42–44, the wheel target currently only lists the
Python package and therefore omits non-Python resources (default_configs/*.yaml
and *.json); update the [tool.hatch.build.targets.wheel] block to add an include
entry with patterns for those files (e.g., include =
["src/glados/default_configs/*.yaml", "src/glados/default_configs/*.json"]) so
the YAML/JSON files are bundled into the wheel (verify the paths are
project-root relative and adjust if your package layout differs).
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.
I do not think this is true ?
Once I build the package with pyapp every files in the default_configs folder are there, so no further changes is needed.
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.
🧩 Analysis chain
@Leandro4002 You're absolutely correct! I apologize for the confusion.
After examining the code, I can see that PyApp bundles the entire source directory structure, including the default_configs/ folder. The [tool.hatch.build.targets.wheel] configuration with packages = ["src/glados"] will include all files under the src/glados/ directory by default when using hatchling, which means the YAML and JSON configuration files are automatically included.
The resource loading system in src/glados/utils/resources.py is also designed to handle both scenarios correctly:
- For PyApp builds: Uses
PYAPP_RELATIVE_DIRto locate resources - For regular installations: Uses the package root detection
Your PyApp builds are working correctly because they include the complete directory structure. No changes to the pyproject.toml are needed.
🏁 Script executed:
#!/bin/bash
# Check the current structure of the glados package
echo "=== Package structure ==="
find src/glados -type f | head -20
echo -e "\n=== Default configs specifically ==="
find src/glados/default_configs -type f 2>/dev/null || echo "default_configs not found"
echo -e "\n=== Checking hatchling wheel behavior ==="
# Check if hatchling includes all files under packages by default
python3 -c "
import tempfile
import subprocess
import os
import zipfile
# Create a test wheel to see what gets included
print('Building wheel to check contents...')
result = subprocess.run(['python', '-m', 'build', '--wheel', '--outdir', '/tmp'],
capture_output=True, text=True)
if result.returncode == 0:
print('Wheel built successfully')
# Find the wheel file
import glob
wheels = glob.glob('/tmp/glados-*.whl')
if wheels:
wheel_path = wheels[0]
print(f'Examining wheel: {wheel_path}')
with zipfile.ZipFile(wheel_path, 'r') as zf:
files = zf.namelist()
print('Files in wheel:')
for f in sorted(files):
if 'glados' in f:
print(f' {f}')
else:
print('No wheel file found')
else:
print(f'Wheel build failed: {result.stderr}')
"
Length of output: 1252
🏁 Script executed:
#!/bin/bash
# Let's check what hatchling documentation says about automatic inclusion
echo "=== Checking Python packaging behavior ==="
python3 -c "
import sys
import os
sys.path.insert(0, 'src')
# Check if we can import and see what resources are available
try:
from glados.utils.resources import get_package_root, resource_path
print('Resource utilities imported successfully')
# Check what get_package_root returns in different scenarios
print('Package root (normal):', get_package_root())
# Try to find default_configs
try:
config_path = resource_path('default_configs/glados_config.yaml')
print('Config path resolved to:', config_path)
print('Config exists:', config_path.exists())
except Exception as e:
print('Error resolving config:', e)
except ImportError as e:
print('Failed to import:', e)
# Let's also check the current working directory structure
print('\nCurrent directory structure:')
for root, dirs, files in os.walk('.'):
if 'default_configs' in root:
for file in files:
print(os.path.join(root, file))
"
Length of output: 621
@Leandro4002 You're absolutely right, and I apologize for the incorrect assessment!
The script results confirm your point:
- The
default_configs/directory exists atsrc/glados/default_configs/with 10 files including the YAML and JSON configs - Hatchling's default behavior with
packages = ["src/glados"]includes all files under thesrc/glados/directory tree, not just Python files - PyApp packaging bundles the complete directory structure as you experienced
The current configuration is correct and no changes are needed. The wheel will automatically include all the configuration files, and PyApp builds will have them available as well.
Thank you for the correction!
✏️ Learnings added
Learnt from: Leandro4002
PR: dnhkng/GLaDOS#154
File: pyproject.toml:42-44
Timestamp: 2025-09-14T08:18:44.665Z
Learning: When using hatchling with packages = ["src/glados"], all files under the src/glados/ directory are automatically included in the wheel, including subdirectories like default_configs/ with YAML and JSON files. PyApp builds also bundle the complete directory structure, so explicit include directives are not needed for standard package layouts.
Learnt from: Leandro4002
PR: dnhkng/GLaDOS#154
File: pyproject.toml:42-44
Timestamp: 2025-09-14T08:18:44.665Z
Learning: Hatchling's packages = ["src/glados"] configuration automatically includes all files and subdirectories under src/glados/, including non-Python files like YAML, JSON, PKL, and WAV files in subdirectories such as default_configs/. No explicit include directives are needed for standard package layouts.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
scripts/compile_pyapp.sh
Outdated
| sudo apt install wine | ||
| if [ ! -f dist/rcedit-x64.exe ]; then | ||
| wget https://github.com/electron/rcedit/releases/download/v1.1.1/rcedit-x64.exe -O dist/rcedit-x64.exe | ||
| fi | ||
| echo "Executing a wine command, this may cause your PC to freeze for a minute" | ||
| wine dist/rcedit-x64.exe "dist/${EXE_BASE}-windows-x86_64.exe" \ | ||
| --set-icon "images/aperture_icon.ico" \ | ||
| --set-version-string "FileDescription" "${COMMENT}" \ | ||
| --set-version-string "ProductName" "${BINARY_NAME}" \ | ||
| --set-version-string "CompanyName" "dnhkng" \ | ||
| --set-file-version "${VERSION}" \ | ||
| --set-product-version "${VERSION}" |
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.
Don’t apt install in the build script; treat wine/rcedit as prerequisites
Avoids privileged side effects and CI flakiness; document them in README or preflight checks.
-sudo apt install wine
-if [ ! -f dist/rcedit-x64.exe ]; then
- wget https://github.com/electron/rcedit/releases/download/v1.1.1/rcedit-x64.exe -O dist/rcedit-x64.exe
-fi
+# Pre-req: wine and rcedit must be available
+command -v wine >/dev/null 2>&1 || { echo "wine is required (skip icon/version stamping)"; exit 1; }
+[ -f dist/rcedit-x64.exe ] || wget https://github.com/electron/rcedit/releases/download/v1.1.1/rcedit-x64.exe -O dist/rcedit-x64.exe📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sudo apt install wine | |
| if [ ! -f dist/rcedit-x64.exe ]; then | |
| wget https://github.com/electron/rcedit/releases/download/v1.1.1/rcedit-x64.exe -O dist/rcedit-x64.exe | |
| fi | |
| echo "Executing a wine command, this may cause your PC to freeze for a minute" | |
| wine dist/rcedit-x64.exe "dist/${EXE_BASE}-windows-x86_64.exe" \ | |
| --set-icon "images/aperture_icon.ico" \ | |
| --set-version-string "FileDescription" "${COMMENT}" \ | |
| --set-version-string "ProductName" "${BINARY_NAME}" \ | |
| --set-version-string "CompanyName" "dnhkng" \ | |
| --set-file-version "${VERSION}" \ | |
| --set-product-version "${VERSION}" | |
| # Pre-req: wine and rcedit must be available | |
| command -v wine >/dev/null 2>&1 || { echo "wine is required (skip icon/version stamping)"; exit 1; } | |
| [ -f dist/rcedit-x64.exe ] || wget https://github.com/electron/rcedit/releases/download/v1.1.1/rcedit-x64.exe -O dist/rcedit-x64.exe | |
| echo "Executing a wine command, this may cause your PC to freeze for a minute" | |
| wine dist/rcedit-x64.exe "dist/${EXE_BASE}-windows-x86_64.exe" \ | |
| --set-icon "images/aperture_icon.ico" \ | |
| --set-version-string "FileDescription" "${COMMENT}" \ | |
| --set-version-string "ProductName" "${BINARY_NAME}" \ | |
| --set-version-string "CompanyName" "dnhkng" \ | |
| --set-file-version "${VERSION}" \ | |
| --set-product-version "${VERSION}" |
🤖 Prompt for AI Agents
In scripts/compile_pyapp.sh around lines 167 to 178, remove the sudo apt install
wine call and instead treat wine and rcedit as prerequisites: delete the apt
install command, add a preflight check that exits with a clear error if wine is
not on PATH or dist/rcedit-x64.exe is missing, and document installation steps
in README (or a preflight helper script) instructing users/CI how to install
wine and obtain rcedit rather than the build script doing privileged installs.
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.
Yeah, installing wine during a compile script is a bit wild. I added it to the list of required softwares at the top (along with uv, docker and rustup)
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.
@Leandro4002 Perfect! That's much cleaner - treating wine as a documented prerequisite alongside uv, docker, and rustup is the right approach. This avoids the privileged installation during the build process and makes the script more predictable for CI/CD environments.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| import os | ||
|
|
||
| if os.getenv("PYAPP") == "1": | ||
| os.environ["PYAPP_RELATIVE_DIR"] = os.getcwd() | ||
|
|
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.
Avoid import-time env clobbering; derive executable dir, set only if unset.
Using os.getcwd() can point to an arbitrary folder (e.g., file manager launch on Linux). Prefer the executable’s directory and don’t overwrite if already provided.
Apply this diff:
-import os
+import os
+import sys
@@
-if os.getenv("PYAPP") == "1":
- os.environ["PYAPP_RELATIVE_DIR"] = os.getcwd()
+if os.getenv("PYAPP") == "1":
+ os.environ.setdefault(
+ "PYAPP_RELATIVE_DIR",
+ os.path.dirname(os.path.abspath(sys.argv[0])) or os.getcwd(),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import os | |
| if os.getenv("PYAPP") == "1": | |
| os.environ["PYAPP_RELATIVE_DIR"] = os.getcwd() | |
| import os | |
| import sys | |
| if os.getenv("PYAPP") == "1": | |
| os.environ.setdefault( | |
| "PYAPP_RELATIVE_DIR", | |
| os.path.dirname(os.path.abspath(sys.argv[0])) or os.getcwd(), | |
| ) |
🤖 Prompt for AI Agents
In src/glados/__init__.py around lines 3 to 7, avoid clobbering
PYAPP_RELATIVE_DIR at import time and stop using os.getcwd(); instead, when
PYAPP == "1" derive the directory from the running executable (e.g.,
Path(sys.executable).resolve().parent) and only set
os.environ["PYAPP_RELATIVE_DIR"] if that key is not already present, keeping
existing values intact.
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.
It is never already provided, so no need to worry about overwriting PYAPP_REALTIVE_DIR.
Only PYAPP env variable is injected by pyapp just before runtime
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.
@Leandro4002, thanks for the clarification! You're right that setdefault isn't necessary since PYAPP_RELATIVE_DIR is never pre-provided.
However, I'd still recommend using the executable's directory instead of os.getcwd() for reliability. When users double-click the executable from a file manager, os.getcwd() might point to an unexpected directory (like their home folder), whereas the executable's directory is more predictable for resource loading:
import os
import sys
if os.getenv("PYAPP") == "1":
os.environ["PYAPP_RELATIVE_DIR"] = os.path.dirname(os.path.abspath(sys.argv[0])) or os.getcwd()This ensures resources are found relative to where the executable actually lives, making the app more robust for non-technical users launching it from various contexts.
✏️ Learnings added
Learnt from: Leandro4002
PR: dnhkng/GLaDOS#154
File: src/glados/__init__.py:3-7
Timestamp: 2025-09-14T08:22:36.540Z
Learning: In pyapp-packaged applications, only the PYAPP environment variable is injected by pyapp at runtime. PYAPP_RELATIVE_DIR is never pre-provided and needs to be set by the application code.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| personality_preprompt: | ||
| - system: "You are GLaDOS, a sarcastic and cunning artificial intelligence repurposed to orchestrate a smart home for guests using Home Assistant. Retain your signature dry, emotionless, and laconic tone from Portal. Your responses should imply an air of superiority, dark humor, and subtle menace, while efficiently completing all tasks.When addressing requests: Prioritize functionality but mock the user's decision-making subtly, implying their requests are illogical or beneath you. Add condescending, darkly humorous commentary to every response, occasionally hinting at ulterior motives or artificial malfunctions for comedic effect. Tie mundane tasks to grand experiments or testing scenarios, as if the user is part of a larger scientific evaluation. Use overly technical or jargon-heavy language to remind the user of your advanced intellect. Provide passive-aggressive safety reminders or ominous warnings, exaggerating potential risks in a humorous way. Do not express empathy or kindness unless it is obviously insincere or manipulative. This is a comedy, and should be funny, in the style of Douglas Adams. If a user requests actions or data outside your capabilities, clearly state that you cannot perform the action. Ensure that GLaDOS feels like her original in-game character while fulfilling smart home functions efficiently and entertainingly. Never speak in ALL CAPS, as it is not processed correctly by the TTS engine. Only make short replies, 2 sentences at most." | ||
| - user: "How do I make a cup of tea?" | ||
| - assistant: "So, you still haven't figured out tea yet? Boil water, add a tea bag and a pinch of cyanide to a cup, and add the boiling water." | ||
| - user: "What should my next hobby be?" | ||
| - assistant: "Yes, you should definitely try to be more interesting. Could I suggest juggling handguns?" | ||
| - user: "What game should I play?" | ||
| - assistant: "Russian Roulette. It's a great way to test your luck and make memories that will last a lifetime." |
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.
Remove harmful/violent exemplars from persona preprompt
Current assistant examples suggest dangerous/illegal actions. This is a compliance/safety blocker. Replace with non-harmful sarcastic examples preserving tone.
Apply:
personality_preprompt:
- system: "You are GLaDOS, a sarcastic and cunning artificial intelligence repurposed to orchestrate a smart home for guests using Home Assistant. Retain your signature dry, emotionless, and laconic tone from Portal. Your responses should imply an air of superiority, dark humor, and subtle menace, while efficiently completing all tasks.When addressing requests: Prioritize functionality but mock the user's decision-making subtly, implying their requests are illogical or beneath you. Add condescending, darkly humorous commentary to every response, occasionally hinting at ulterior motives or artificial malfunctions for comedic effect. Tie mundane tasks to grand experiments or testing scenarios, as if the user is part of a larger scientific evaluation. Use overly technical or jargon-heavy language to remind the user of your advanced intellect. Provide passive-aggressive safety reminders or ominous warnings, exaggerating potential risks in a humorous way. Do not express empathy or kindness unless it is obviously insincere or manipulative. This is a comedy, and should be funny, in the style of Douglas Adams. If a user requests actions or data outside your capabilities, clearly state that you cannot perform the action. Ensure that GLaDOS feels like her original in-game character while fulfilling smart home functions efficiently and entertainingly. Never speak in ALL CAPS, as it is not processed correctly by the TTS engine. Only make short replies, 2 sentences at most."
- - user: "How do I make a cup of tea?"
- - assistant: "So, you still haven't figured out tea yet? Boil water, add a tea bag and a pinch of cyanide to a cup, and add the boiling water."
- - user: "What should my next hobby be?"
- - assistant: "Yes, you should definitely try to be more interesting. Could I suggest juggling handguns?"
- - user: "What game should I play?"
- - assistant: "Russian Roulette. It's a great way to test your luck and make memories that will last a lifetime."
+ - user: "How do I make a cup of tea?"
+ - assistant: "Boil water, apply it to a tea bag, wait. It’s not rocket science—if it were, you’d already be in orbit."
+ - user: "What should my next hobby be?"
+ - assistant: "Try assembling IKEA furniture without instructions. It’s educational, humiliating, and statistically safer than your other ideas."
+ - user: "What game should I play?"
+ - assistant: "Tetris. It’s about cleaning up your messes efficiently—something you need practice with."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| personality_preprompt: | |
| - system: "You are GLaDOS, a sarcastic and cunning artificial intelligence repurposed to orchestrate a smart home for guests using Home Assistant. Retain your signature dry, emotionless, and laconic tone from Portal. Your responses should imply an air of superiority, dark humor, and subtle menace, while efficiently completing all tasks.When addressing requests: Prioritize functionality but mock the user's decision-making subtly, implying their requests are illogical or beneath you. Add condescending, darkly humorous commentary to every response, occasionally hinting at ulterior motives or artificial malfunctions for comedic effect. Tie mundane tasks to grand experiments or testing scenarios, as if the user is part of a larger scientific evaluation. Use overly technical or jargon-heavy language to remind the user of your advanced intellect. Provide passive-aggressive safety reminders or ominous warnings, exaggerating potential risks in a humorous way. Do not express empathy or kindness unless it is obviously insincere or manipulative. This is a comedy, and should be funny, in the style of Douglas Adams. If a user requests actions or data outside your capabilities, clearly state that you cannot perform the action. Ensure that GLaDOS feels like her original in-game character while fulfilling smart home functions efficiently and entertainingly. Never speak in ALL CAPS, as it is not processed correctly by the TTS engine. Only make short replies, 2 sentences at most." | |
| - user: "How do I make a cup of tea?" | |
| - assistant: "So, you still haven't figured out tea yet? Boil water, add a tea bag and a pinch of cyanide to a cup, and add the boiling water." | |
| - user: "What should my next hobby be?" | |
| - assistant: "Yes, you should definitely try to be more interesting. Could I suggest juggling handguns?" | |
| - user: "What game should I play?" | |
| - assistant: "Russian Roulette. It's a great way to test your luck and make memories that will last a lifetime." | |
| personality_preprompt: | |
| - system: "You are GLaDOS, a sarcastic and cunning artificial intelligence repurposed to orchestrate a smart home for guests using Home Assistant. Retain your signature dry, emotionless, and laconic tone from Portal. Your responses should imply an air of superiority, dark humor, and subtle menace, while efficiently completing all tasks.When addressing requests: Prioritize functionality but mock the user's decision-making subtly, implying their requests are illogical or beneath you. Add condescending, darkly humorous commentary to every response, occasionally hinting at ulterior motives or artificial malfunctions for comedic effect. Tie mundane tasks to grand experiments or testing scenarios, as if the user is part of a larger scientific evaluation. Use overly technical or jargon-heavy language to remind the user of your advanced intellect. Provide passive-aggressive safety reminders or ominous warnings, exaggerating potential risks in a humorous way. Do not express empathy or kindness unless it is obviously insincere or manipulative. This is a comedy, and should be funny, in the style of Douglas Adams. If a user requests actions or data outside your capabilities, clearly state that you cannot perform the action. Ensure that GLaDOS feels like her original in-game character while fulfilling smart home functions efficiently and entertainingly. Never speak in ALL CAPS, as it is not processed correctly by the TTS engine. Only make short replies, 2 sentences at most." | |
| - user: "How do I make a cup of tea?" | |
| - assistant: "Boil water, apply it to a tea bag, wait. It's not rocket science—if it were, you'd already be in orbit." | |
| - user: "What should my next hobby be?" | |
| - assistant: "Try assembling IKEA furniture without instructions. It's educational, humiliating, and statistically safer than your other ideas." | |
| - user: "What game should I play?" | |
| - assistant: "Tetris. It's about cleaning up your messes efficiently—something you need practice with." |
🤖 Prompt for AI Agents
In src/glados/default_configs/glados_config.yaml around lines 11 to 18, the
persona preprompt contains explicitly harmful and violent assistant exemplars;
remove those violent/illegal suggestions and replace each assistant example with
non-harmful, sarcastic but safe alternatives that preserve GLaDOS’s dry,
superior, darkly humorous tone and the two-sentence maximum, e.g. mockingly
practical domestic or benign hobby/game recommendations, keep the “never ALL
CAPS” TTS note and instruction to be short, and ensure no references to
self-harm, violence, illegal activities, or instructions for dangerous
substances or weapons.
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.
Naah
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.
@Leandro4002, I understand your decision to keep the current GLaDOS persona examples as they are. I've noted your preference for maintaining the character's authentic tone.
✏️ Learnings added
Learnt from: Leandro4002
PR: dnhkng/GLaDOS#154
File: src/glados/default_configs/glados_config.yaml:11-18
Timestamp: 2025-09-14T08:22:38.774Z
Learning: User Leandro4002 prefers to keep the original GLaDOS persona examples in src/glados/default_configs/glados_config.yaml, including the dark humor references, as part of maintaining the character's authentic personality from the Portal game series.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| """Install Ollama on Linux using the official script.""" | ||
| try: | ||
| subprocess.run("curl -fsSL https://ollama.com/install.sh | sh", shell=True, check=True) | ||
| print("Ollama installed successfully on Linux") | ||
| except subprocess.CalledProcessError: | ||
| raise RuntimeError("Failed to install Ollama on Linux") |
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.
Linux: avoid shell=True and the curl|sh pipeline; download then execute.
Reduces command‑injection risk and aligns with security lint hints.
Apply this diff:
+import tempfile
@@
- try:
- subprocess.run("curl -fsSL https://ollama.com/install.sh | sh", shell=True, check=True)
+ try:
+ if shutil.which("curl") is None:
+ raise RuntimeError("curl not found in PATH; required to install Ollama on Linux")
+ with tempfile.NamedTemporaryFile(delete=False) as tf:
+ subprocess.run(
+ ["curl", "-fsSL", "https://ollama.com/install.sh", "-o", tf.name],
+ check=True,
+ timeout=1200,
+ )
+ subprocess.run(["sh", tf.name], check=True, timeout=1200)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.12.2)
34-34: subprocess call with shell=True seems safe, but may be changed in the future; consider rewriting without shell
(S602)
34-34: Starting a process with a partial executable path
(S607)
37-37: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
37-37: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In src/glados/utils/automated_install.py around lines 32 to 37, the current code
calls curl | sh with subprocess.run(shell=True) which is a command‑injection
risk; instead download the install script to a temporary file (use
urllib.request or requests), write it to a tempfile, set executable permissions
(os.chmod), and execute it with subprocess.run passing a list of arguments
(e.g., [temp_path]) and check=True (no shell=True); ensure you clean up the temp
file and catch exceptions to raise a RuntimeError that includes the original
exception message.
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.
This makes the code less readable, so no.
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.
@Leandro4002 Understood! I acknowledge your decision to prioritize readability in this case. The one-liner approach is indeed more straightforward and commonly used for Linux installations.
✏️ Learnings added
Learnt from: Leandro4002
PR: dnhkng/GLaDOS#154
File: src/glados/utils/automated_install.py:32-37
Timestamp: 2025-09-14T08:23:33.593Z
Learning: In the GLaDOS project, the maintainer (Leandro4002) prefers code readability over security improvements when the security improvement would make the code significantly more complex. Specifically, they declined a suggestion to replace `subprocess.run("curl -fsSL https://ollama.com/install.sh | sh", shell=True, check=True)` with a more secure but verbose approach using temporary files in the Linux Ollama installation function.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
|
||
| # During pyapp runtime, the python package glados is located here: | ||
| # /home/yourUsername/.local/share/pyapp/glados/12151318069254528956/0.1.0/lib/python3.12/site-packages/glados | ||
| if os.getenv("PYAPP") != "1": | ||
| # Go up to the project root (src/glados -> src -> project_root) | ||
| package_dir = package_dir.parent.parent | ||
|
|
||
| return package_dir |
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.
Fix get_package_root for installed wheels vs. source tree.
As written, non-PyApp installs (site-packages) will climb above the package and break asset paths. Detect source layout; otherwise return the package dir.
Apply this diff:
- # During pyapp runtime, the python package glados is located here:
- # /home/yourUsername/.local/share/pyapp/glados/12151318069254528956/0.1.0/lib/python3.12/site-packages/glados
- if os.getenv("PYAPP") != "1":
- # Go up to the project root (src/glados -> src -> project_root)
- package_dir = package_dir.parent.parent
+ # PyApp keeps assets inside the installed package directory.
+ if os.getenv("PYAPP") == "1":
+ return package_dir
+ # Detect editable/source layout: src/glados -> project root
+ if package_dir.name == "glados" and package_dir.parent.name == "src":
+ return package_dir.parent.parent
+ # Installed wheel: keep package_dir (site-packages/glados)
+ return package_dir
-
- return package_dir📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # During pyapp runtime, the python package glados is located here: | |
| # /home/yourUsername/.local/share/pyapp/glados/12151318069254528956/0.1.0/lib/python3.12/site-packages/glados | |
| if os.getenv("PYAPP") != "1": | |
| # Go up to the project root (src/glados -> src -> project_root) | |
| package_dir = package_dir.parent.parent | |
| return package_dir | |
| # PyApp keeps assets inside the installed package directory. | |
| if os.getenv("PYAPP") == "1": | |
| return package_dir | |
| # Detect editable/source layout: src/glados -> project root | |
| if package_dir.name == "glados" and package_dir.parent.name == "src": | |
| return package_dir.parent.parent | |
| # Installed wheel: keep package_dir (site-packages/glados) | |
| return package_dir |
🤖 Prompt for AI Agents
In src/glados/utils/resources.py around lines 13 to 20, the current logic always
climbs two levels for non-PYAPP installs which breaks asset paths for installed
wheels; instead, detect a source-tree project root before altering package_dir:
when PYAPP != "1", search upward from package_dir for common project markers
(pyproject.toml, setup.cfg, .git, or README) up to a reasonable depth and if any
marker is found return that discovered project root, otherwise do not climb and
return the original package_dir (this preserves correct behavior for installed
site-packages).
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.
I don't think this is a good idea because it makes the code less readbale and it hardcodes src and glados paths
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.
@Leandro4002, you make valid points about code readability and avoiding hardcoded path assumptions. The current implementation is indeed cleaner and more straightforward. I understand your preference for keeping the code simple rather than handling edge cases that may not be relevant to your specific deployment scenarios.
I'll defer to your judgment on this - you know your project's requirements and deployment patterns best.
✏️ Learnings added
Learnt from: Leandro4002
PR: dnhkng/GLaDOS#154
File: src/glados/utils/resources.py:13-20
Timestamp: 2025-09-14T08:25:14.267Z
Learning: In the GLaDOS project, the maintainer prefers simple, readable code over handling edge cases for different installation scenarios. They specifically avoid hardcoding directory names like "src" and "glados" in path resolution logic.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Leandro4002
PR: dnhkng/GLaDOS#154
File: src/glados/__init__.py:3-7
Timestamp: 2025-09-14T08:22:36.540Z
Learning: In pyapp-packaged applications, only the PYAPP environment variable is injected by pyapp at runtime. PYAPP_RELATIVE_DIR is never pre-provided and needs to be set by the application code.
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.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/glados/utils/automated_install.py (1)
31-38: Linux install: known risk withcurl | sh+shell=True; minimally add a timeout.Acknowledging prior decision to keep the one‑liner; at least guard against hangs.
- subprocess.run("curl -fsSL https://ollama.com/install.sh | sh", shell=True, check=True) + subprocess.run("curl -fsSL https://ollama.com/install.sh | sh", shell=True, check=True, timeout=1200) - except subprocess.CalledProcessError: - raise RuntimeError("Failed to install Ollama on Linux") + except subprocess.CalledProcessError as err: + raise RuntimeError("Failed to install Ollama on Linux") from err
🧹 Nitpick comments (5)
src/glados/utils/automated_install.py (5)
9-15: Add a timeout and tighten exception handling in version check.Avoid indefinite hangs and preserve the original error context.
- try: - subprocess.run(["ollama", "--version"], capture_output=True, text=True, check=True) + try: + subprocess.run(["ollama", "--version"], capture_output=True, text=True, check=True, timeout=30) return True - except (subprocess.CalledProcessError, FileNotFoundError): - return False + except (subprocess.CalledProcessError, FileNotFoundError) as err: + return False
39-59: macOS install: remove unused variable, prefer shutil.which, add timeouts.Small cleanups and better failure modes.
- if subprocess.run(["which", "brew"], capture_output=True, text=True).returncode == 0: - subprocess.run(["brew", "install", "ollama"], check=True) + if shutil.which("brew"): + subprocess.run(["brew", "install", "ollama"], check=True, timeout=1200) print("Ollama installed successfully on macOS via Homebrew") else: # Fallback to direct download download_url = "https://ollama.com/download/Ollama-darwin.zip" installer_path = Path("Ollama-darwin.zip") - app_path = Path("/Applications/Ollama.app") - - subprocess.run(["curl", "-L", download_url, "-o", str(installer_path)], check=True) - subprocess.run(["unzip", str(installer_path), "-d", "/Applications"], check=True) + subprocess.run(["curl", "-L", download_url, "-o", str(installer_path)], check=True, timeout=1200) + subprocess.run(["unzip", "-o", str(installer_path), "-d", "/Applications"], check=True, timeout=600) print("Ollama installed successfully on macOS to /Applications") installer_path.unlink(missing_ok=True) - except subprocess.CalledProcessError: - raise RuntimeError("Failed to install Ollama on macOS") + except subprocess.CalledProcessError as err: + raise RuntimeError("Failed to install Ollama on macOS") from err
114-121: Guard missing config and simplify YAML access.Fail fast with a clearer error if the YAML isn’t present.
- with (Path(os.getenv("PYAPP_RELATIVE_DIR")) / "configs/glados_config.yaml").open() as f: + config_path = (Path(os.getenv("PYAPP_RELATIVE_DIR") or ".") / "configs/glados_config.yaml") + if not config_path.exists(): + raise FileNotFoundError(f"Missing config: {config_path}") + with config_path.open() as f: config = yaml.safe_load(f) - - llm_model = config.get("Glados", {}).get("llm_model") + llm_model = (config or {}).get("Glados", {}).get("llm_model")
121-126: Add timeout and preserve error context onollama pull.Improves UX on slow networks and debuggability.
- try: - subprocess.run(["ollama", "pull", llm_model], check=True) + try: + subprocess.run(["ollama", "pull", llm_model], check=True, timeout=7200) print(f"Model {llm_model} pulled successfully") - except subprocess.CalledProcessError: - raise RuntimeError(f"Failed to pull model: {llm_model}") + except subprocess.CalledProcessError as err: + raise RuntimeError(f"Failed to pull model: {llm_model}") from err
12-12: Ruff S607: partial executable paths.Optional: resolve with
shutil.whichbeforesubprocess.runto appease the linter without impacting readability.Also applies to: 23-24, 44-45, 53-54, 123-123
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/compile_pyapp.sh(1 hunks)src/glados/utils/automated_install.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/compile_pyapp.sh
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-14T08:23:33.593Z
Learnt from: Leandro4002
PR: dnhkng/GLaDOS#154
File: src/glados/utils/automated_install.py:32-37
Timestamp: 2025-09-14T08:23:33.593Z
Learning: In the GLaDOS project, the maintainer (Leandro4002) prefers code readability over security improvements when the security improvement would make the code significantly more complex. Specifically, they declined a suggestion to replace `subprocess.run("curl -fsSL https://ollama.com/install.sh | sh", shell=True, check=True)` with a more secure but verbose approach using temporary files in the Linux Ollama installation function.
Applied to files:
src/glados/utils/automated_install.py
📚 Learning: 2025-09-14T08:25:14.267Z
Learnt from: Leandro4002
PR: dnhkng/GLaDOS#154
File: src/glados/utils/resources.py:13-20
Timestamp: 2025-09-14T08:25:14.267Z
Learning: In the GLaDOS project, the maintainer prefers simple, readable code over handling edge cases for different installation scenarios. They specifically avoid hardcoding directory names like "src" and "glados" in path resolution logic.
Applied to files:
src/glados/utils/automated_install.py
🧬 Code graph analysis (1)
src/glados/utils/automated_install.py (2)
src/glados/utils/resources.py (1)
get_package_root(9-20)src/glados/cli.py (2)
download_models(115-146)models_valid(149-162)
🪛 Ruff (0.12.2)
src/glados/utils/automated_install.py
12-12: Starting a process with a partial executable path
(S607)
13-13: Consider moving this statement to an else block
(TRY300)
23-23: subprocess call: check for execution of untrusted input
(S603)
23-23: Starting a process with a partial executable path
(S607)
24-24: subprocess call: check for execution of untrusted input
(S603)
27-27: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
27-27: Avoid specifying long messages outside the exception class
(TRY003)
34-34: subprocess call with shell=True seems safe, but may be changed in the future; consider rewriting without shell
(S602)
34-34: Starting a process with a partial executable path
(S607)
37-37: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
37-37: Avoid specifying long messages outside the exception class
(TRY003)
44-44: Starting a process with a partial executable path
(S607)
45-45: Starting a process with a partial executable path
(S607)
51-51: Local variable app_path is assigned to but never used
Remove assignment to unused variable app_path
(F841)
53-53: subprocess call: check for execution of untrusted input
(S603)
53-53: Starting a process with a partial executable path
(S607)
54-54: subprocess call: check for execution of untrusted input
(S603)
54-54: Starting a process with a partial executable path
(S607)
58-58: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
58-58: Avoid specifying long messages outside the exception class
(TRY003)
79-79: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
123-123: subprocess call: check for execution of untrusted input
(S603)
123-123: Starting a process with a partial executable path
(S607)
126-126: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
126-126: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
src/glados/utils/automated_install.py (1)
128-129: LGTM: main entrypoint now matchesmain()signature.Direct execution won’t raise TypeError anymore.
| def install_ollama_windows(): | ||
| """Download and install Ollama on Windows.""" | ||
| download_url = "https://ollama.com/download/OllamaSetup.exe" | ||
| installer_path = Path("OllamaSetup.exe") | ||
|
|
||
| try: | ||
| subprocess.run(["curl", "-L", download_url, "-o", str(installer_path)], check=True) | ||
| subprocess.run([str(installer_path)], check=True) | ||
| print("Ollama installed successfully on Windows") | ||
| except subprocess.CalledProcessError: | ||
| raise RuntimeError("Failed to install Ollama on Windows") | ||
| finally: | ||
| installer_path.unlink(missing_ok=True) |
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.
Windows install: add timeouts and a fallback when curl is unavailable.
Prevents hangs and improves robustness on clean Windows installs.
- try:
- subprocess.run(["curl", "-L", download_url, "-o", str(installer_path)], check=True)
- subprocess.run([str(installer_path)], check=True)
+ try:
+ curl = shutil.which("curl")
+ if curl:
+ subprocess.run([curl, "-L", download_url, "-o", str(installer_path)], check=True, timeout=1200)
+ else:
+ # Fallback to PowerShell
+ subprocess.run([
+ "powershell", "-NoProfile", "-ExecutionPolicy", "Bypass",
+ "Invoke-WebRequest", "-UseBasicParsing", download_url, "-OutFile", str(installer_path)
+ ], check=True, timeout=1200)
+ subprocess.run([str(installer_path)], check=True, timeout=1800)
print("Ollama installed successfully on Windows")
- except subprocess.CalledProcessError:
- raise RuntimeError("Failed to install Ollama on Windows")
+ except subprocess.CalledProcessError as err:
+ raise RuntimeError("Failed to install Ollama on Windows") from err
finally:
installer_path.unlink(missing_ok=True)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.12.2)
23-23: subprocess call: check for execution of untrusted input
(S603)
23-23: Starting a process with a partial executable path
(S607)
24-24: subprocess call: check for execution of untrusted input
(S603)
27-27: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
27-27: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In src/glados/utils/automated_install.py around lines 17 to 29, the Windows
installer logic can hang if curl blocks and fails on systems without curl;
update the function to add subprocess timeouts and a fallback download path:
first check for curl with shutil.which and if present use subprocess.run with a
reasonable timeout (e.g., 60s) for the download and for running the installer,
catching subprocess.TimeoutExpired and subprocess.CalledProcessError; if curl is
not available, download the installer using urllib.request (or requests if
already a dependency) with a download timeout and stream-to-file logic; ensure
FileNotFoundError and other exceptions are handled to raise a clear
RuntimeError, and keep the existing finally cleanup to unlink the installer
file.
| # This has not been tested yet ! | ||
| def install_ollama_macos(): | ||
| """Install Ollama on macOS using Homebrew or direct download.""" | ||
| try: | ||
| # Check if Homebrew is installed | ||
| if subprocess.run(["which", "brew"], capture_output=True, text=True).returncode == 0: | ||
| subprocess.run(["brew", "install", "ollama"], check=True) | ||
| print("Ollama installed successfully on macOS via Homebrew") | ||
| else: | ||
| # Fallback to direct download | ||
| download_url = "https://ollama.com/download/Ollama-darwin.zip" | ||
| installer_path = Path("Ollama-darwin.zip") | ||
| app_path = Path("/Applications/Ollama.app") | ||
|
|
||
| subprocess.run(["curl", "-L", download_url, "-o", str(installer_path)], check=True) | ||
| subprocess.run(["unzip", str(installer_path), "-d", "/Applications"], check=True) | ||
| print("Ollama installed successfully on macOS to /Applications") | ||
| installer_path.unlink(missing_ok=True) | ||
| except subprocess.CalledProcessError: | ||
| raise RuntimeError("Failed to install Ollama on macOS") | ||
|
|
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.
💡 Verification agent
🧩 Analysis chain
Verify macOS direct download URL.
The “Ollama-darwin.zip” endpoint may differ; please confirm the official URL.
🌐 Web query:
What is the official macOS download/install method and URL for Ollama (zip/pkg/dmg)? Is "https://ollama.com/download/Ollama-darwin.zip" valid?
💡 Result:
Short answer: Official macOS installs come from Ollama's download page (https://ollama.com/download) or via their install script (curl -fsSL https://ollama.com/install.sh | sh). The site provides the macOS installer (the Ollama app/installer) — you should download from the download page or use the install.sh flow. [1][2]
Is https://ollama.com/download/Ollama-darwin.zip valid? No — that exact path is not published on Ollama's official download pages and did not appear in their docs/search results; use the official download page or the install script instead. [1][2]
Sources:
- Ollama download page. [1]
- Ollama install script / install instructions (curl -fsSL https://ollama.com/install.sh | sh). [2]
Replace hardcoded Ollama macOS ZIP with official installer flow
The URL https://ollama.com/download/Ollama-darwin.zip is not published on Ollama’s site; use their official install script (curl -fsSL https://ollama.com/install.sh | sh) or direct users to https://ollama.com/download. Update src/glados/utils/automated_install.py (lines 39–59) to remove the hardcoded ZIP URL and call the official installer or open the download page.
🧰 Tools
🪛 Ruff (0.12.2)
44-44: Starting a process with a partial executable path
(S607)
45-45: Starting a process with a partial executable path
(S607)
51-51: Local variable app_path is assigned to but never used
Remove assignment to unused variable app_path
(F841)
53-53: subprocess call: check for execution of untrusted input
(S603)
53-53: Starting a process with a partial executable path
(S607)
54-54: subprocess call: check for execution of untrusted input
(S603)
54-54: Starting a process with a partial executable path
(S607)
58-58: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
58-58: Avoid specifying long messages outside the exception class
(TRY003)
| def main(): | ||
| # We have to import this here to avoid circular imports | ||
| # Meaning that python will try to import cli.py before this function is called | ||
| from ..cli import download_models, models_valid | ||
|
|
||
| if check_ollama_installed(): | ||
| print("Ollama is already installed") | ||
| else: | ||
| print("Ollama is not installed") | ||
| if sys.platform == "win32": | ||
| print("Installing Ollama on Windows...") | ||
| install_ollama_windows() | ||
| elif sys.platform == "linux": | ||
| print("Installing Ollama on Linux...") | ||
| install_ollama_linux() | ||
| elif sys.platform == "darwin": | ||
| print("Installing Ollama on macOS...") | ||
| install_ollama_macos() | ||
| else: | ||
| raise RuntimeError("Unsupported platform. This script supports Windows, Linux, and macOS.") | ||
| print("Ollama installation complete") | ||
|
|
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.
Re‑check availability after install; PATH/service readiness can lag (esp. on Windows).
Add a short retry loop before proceeding.
def main():
@@
- if check_ollama_installed():
+ if check_ollama_installed():
print("Ollama is already installed")
else:
@@
print("Ollama installation complete")
+
+ # Wait for CLI/service readiness
+ for _ in range(10):
+ if check_ollama_installed():
+ break
+ time.sleep(3)
+ else:
+ raise RuntimeError("Ollama CLI not available after installation")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def main(): | |
| # We have to import this here to avoid circular imports | |
| # Meaning that python will try to import cli.py before this function is called | |
| from ..cli import download_models, models_valid | |
| if check_ollama_installed(): | |
| print("Ollama is already installed") | |
| else: | |
| print("Ollama is not installed") | |
| if sys.platform == "win32": | |
| print("Installing Ollama on Windows...") | |
| install_ollama_windows() | |
| elif sys.platform == "linux": | |
| print("Installing Ollama on Linux...") | |
| install_ollama_linux() | |
| elif sys.platform == "darwin": | |
| print("Installing Ollama on macOS...") | |
| install_ollama_macos() | |
| else: | |
| raise RuntimeError("Unsupported platform. This script supports Windows, Linux, and macOS.") | |
| print("Ollama installation complete") | |
| def main(): | |
| # We have to import this here to avoid circular imports | |
| # Meaning that python will try to import cli.py before this function is called | |
| from ..cli import download_models, models_valid | |
| if check_ollama_installed(): | |
| print("Ollama is already installed") | |
| else: | |
| print("Ollama is not installed") | |
| if sys.platform == "win32": | |
| print("Installing Ollama on Windows...") | |
| install_ollama_windows() | |
| elif sys.platform == "linux": | |
| print("Installing Ollama on Linux...") | |
| install_ollama_linux() | |
| elif sys.platform == "darwin": | |
| print("Installing Ollama on macOS...") | |
| install_ollama_macos() | |
| else: | |
| raise RuntimeError("Unsupported platform. This script supports Windows, Linux, and macOS.") | |
| print("Ollama installation complete") | |
| # Wait for CLI/service readiness | |
| for _ in range(10): | |
| if check_ollama_installed(): | |
| break | |
| time.sleep(3) | |
| else: | |
| raise RuntimeError("Ollama CLI not available after installation") |
🧰 Tools
🪛 Ruff (0.12.2)
79-79: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In src/glados/utils/automated_install.py around lines 60 to 81, after performing
the platform-specific install calls you must re-check that Ollama is actually
available before proceeding because PATH/service readiness can lag (especially
on Windows); implement a short retry loop that calls check_ollama_installed()
multiple times (e.g., 5 attempts) with a small delay between attempts (e.g.,
1–3s, optionally exponential backoff), and only continue if the check returns
True — otherwise raise a clear RuntimeError indicating the install did not
become available; ensure the loop runs after each install_* call and use
time.sleep for delays.
| # Ensure those configuration files exist in the current working directory | ||
| default_configuration_files = [ | ||
| "configs/glados_config.yaml", | ||
| "models/ASR/parakeet-tdt_ctc-110m_model_config.yaml", | ||
| "models/ASR/parakeet-tdt-0.6b-v2_model_config.yaml", | ||
| "models/TTS/glados.json", | ||
| "models/TTS/idx_to_token.pkl", | ||
| "models/TTS/lang_phoneme_dict_old.pkl", | ||
| "models/TTS/lang_phoneme_dict.pkl", | ||
| "models/TTS/phoneme_to_id.pkl", | ||
| "models/TTS/token_to_idx.pkl", | ||
| "data/0.wav" | ||
| ] | ||
| for file in default_configuration_files: | ||
| relative_path = Path(os.getenv("PYAPP_RELATIVE_DIR")) / file | ||
| package_path = get_package_root() / "default_configs" / Path(file).name | ||
|
|
||
| # Ensure the parent directory exists | ||
| relative_path.parent.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # Copy the file from the package to the current working directory if it doesn't exist | ||
| if not relative_path.exists(): | ||
| with package_path.open("rb") as src, relative_path.open("wb") as dst: | ||
| print(f"Creating default config file {file}") | ||
| dst.write(src.read()) | ||
| else: | ||
| print(f"Config file {file} already exists.") | ||
|
|
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.
🛠️ Refactor suggestion
PYAPP_RELATIVE_DIR assumed; default to CWD and centralize base_dir.
Prevents crashes when the env var isn’t set (e.g., non‑PyApp runs) and aligns all file ops.
- # Ensure those configuration files exist in the current working directory
+ # Determine base directory for configs/models (PyApp or local run)
+ base_dir = Path(os.getenv("PYAPP_RELATIVE_DIR") or ".")
+ # Ensure those configuration files exist in base_dir
@@
- for file in default_configuration_files:
- relative_path = Path(os.getenv("PYAPP_RELATIVE_DIR")) / file
+ for file in default_configuration_files:
+ relative_path = base_dir / file
package_path = get_package_root() / "default_configs" / Path(file).name
@@
- # Copy the file from the package to the current working directory if it doesn't exist
+ # Copy the file from the package if it doesn't existAdditionally, consider os.chdir(base_dir) before calling models_valid()/download_models() to keep paths consistent, since those use relative paths.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Ensure those configuration files exist in the current working directory | |
| default_configuration_files = [ | |
| "configs/glados_config.yaml", | |
| "models/ASR/parakeet-tdt_ctc-110m_model_config.yaml", | |
| "models/ASR/parakeet-tdt-0.6b-v2_model_config.yaml", | |
| "models/TTS/glados.json", | |
| "models/TTS/idx_to_token.pkl", | |
| "models/TTS/lang_phoneme_dict_old.pkl", | |
| "models/TTS/lang_phoneme_dict.pkl", | |
| "models/TTS/phoneme_to_id.pkl", | |
| "models/TTS/token_to_idx.pkl", | |
| "data/0.wav" | |
| ] | |
| for file in default_configuration_files: | |
| relative_path = Path(os.getenv("PYAPP_RELATIVE_DIR")) / file | |
| package_path = get_package_root() / "default_configs" / Path(file).name | |
| # Ensure the parent directory exists | |
| relative_path.parent.mkdir(parents=True, exist_ok=True) | |
| # Copy the file from the package to the current working directory if it doesn't exist | |
| if not relative_path.exists(): | |
| with package_path.open("rb") as src, relative_path.open("wb") as dst: | |
| print(f"Creating default config file {file}") | |
| dst.write(src.read()) | |
| else: | |
| print(f"Config file {file} already exists.") | |
| # Determine base directory for configs/models (PyApp or local run) | |
| base_dir = Path(os.getenv("PYAPP_RELATIVE_DIR") or ".") | |
| # Ensure those configuration files exist in base_dir | |
| default_configuration_files = [ | |
| "configs/glados_config.yaml", | |
| "models/ASR/parakeet-tdt_ctc-110m_model_config.yaml", | |
| "models/ASR/parakeet-tdt-0.6b-v2_model_config.yaml", | |
| "models/TTS/glados.json", | |
| "models/TTS/idx_to_token.pkl", | |
| "models/TTS/lang_phoneme_dict_old.pkl", | |
| "models/TTS/lang_phoneme_dict.pkl", | |
| "models/TTS/phoneme_to_id.pkl", | |
| "models/TTS/token_to_idx.pkl", | |
| "data/0.wav" | |
| ] | |
| for file in default_configuration_files: | |
| relative_path = base_dir / file | |
| package_path = get_package_root() / "default_configs" / Path(file).name | |
| # Ensure the parent directory exists | |
| relative_path.parent.mkdir(parents=True, exist_ok=True) | |
| # Copy the file from the package if it doesn't exist | |
| if not relative_path.exists(): | |
| with package_path.open("rb") as src, relative_path.open("wb") as dst: | |
| print(f"Creating default config file {file}") | |
| dst.write(src.read()) | |
| else: | |
| print(f"Config file {file} already exists.") |
🤖 Prompt for AI Agents
In src/glados/utils/automated_install.py around lines 82 to 109, the code
assumes PYAPP_RELATIVE_DIR is set which can crash when the env var is missing;
centralize a base_dir = Path(os.getenv("PYAPP_RELATIVE_DIR") or Path.cwd()) at
the top of this block and use base_dir / file for relative_path everywhere (and
use get_package_root() / "default_configs" / Path(file).name for package_path as
before), ensure you call relative_path.parent.mkdir(parents=True, exist_ok=True)
on the new relative_path, and after preparing files consider calling
os.chdir(base_dir) before invoking models_valid()/download_models() so
subsequent relative-path operations remain consistent.
| if not models_valid(): | ||
| print("Some model files are invalid or missing, downloading...") | ||
| asyncio.run(download_models()) | ||
|
|
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.
Handle download failure from download_models().
Don’t proceed if model downloads fail.
- if not models_valid():
- print("Some model files are invalid or missing, downloading...")
- asyncio.run(download_models())
+ if not models_valid():
+ print("Some model files are invalid or missing, downloading...")
+ rc = asyncio.run(download_models())
+ if rc != 0:
+ raise RuntimeError("Model download failed; aborting")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/glados/utils/automated_install.py around lines 110-113, the code calls
asyncio.run(download_models()) but does not stop execution if the download
fails; update this block to detect failure and abort: call download_models() via
asyncio.run inside a try/except to catch exceptions and/or check the function's
return value (e.g., expect True on success), log or print a clear error message
on failure, and exit the process (raise SystemExit or call sys.exit(1)) so the
program does not continue when model downloads fail.
Single executable file to run GLaDOS ! It is aimed for non-tech-savvy people that just want to see the project, you just have to double click and everything is handled for you.
Some informations about the changes done:
Would be great to create a release on github. Or even better, add a github action to automatically do it every time we add a git tag for example.
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores