Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add autoconfig support to installer #170

Merged
merged 11 commits into from
Oct 11, 2024
Merged

Add autoconfig support to installer #170

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7f774b3
idea phase for auoconfig support
suvanbanerjee Oct 6, 2024
9cdb216
Merge branch 'OpenVoiceOS:main' into main
suvanbanerjee Oct 6, 2024
6ae940f
Merge branch 'OpenVoiceOS:main' into main
suvanbanerjee Oct 9, 2024
5a7afae
basic structure
suvanbanerjee Oct 9, 2024
b92337b
Revert "basic structure"
suvanbanerjee Oct 10, 2024
33fa149
clean options
suvanbanerjee Oct 10, 2024
92198b6
[ansible/conf] Handles venv and docker
goldyfruit Oct 10, 2024
fc50a85
[ansible/template] Not required anymore
goldyfruit Oct 10, 2024
d7886b7
add online flag to commands
suvanbanerjee Oct 11, 2024
7e2c383
[ansible/conf] Fix typos
goldyfruit Oct 11, 2024
9423a24
[ansible/conf] Only trigger when profile is not server
goldyfruit Oct 11, 2024
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
7 changes: 7 additions & 0 deletions ansible/roles/ovos_installer/tasks/docker/composer.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,13 @@
command: hivemind-client set-identity --key {{ ovos_installer_satellite_key }} --password {{ ovos_installer_satellite_password }} --host {{ ovos_installer_listener_host }} --port {{ ovos_installer_listener_port | default(5678) }} --siteid {{ ovos_installer_site_id | default("voice-sat-1") }}
when: ovos_installer_profile == "satellite"

- name: Run ovos-config for auto-configuration of STT and TTS based on language
become: true
become_user: "{{ ovos_installer_user }}"
community.docker.docker_container_exec:
container: ovos_cli
command: ovos-config autoconfigure --lang {{ ovos_installer_locale }} --male

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance the auto-configuration task with conditional execution and error handling

The new task for auto-configuration of STT and TTS is a good addition that aligns with the PR objectives. However, consider the following improvements:

  1. Add conditional execution based on the ovos_installer_profile to ensure it only runs for the appropriate setup (e.g., satellite profile).
  2. Implement error handling and output capturing for better troubleshooting capabilities.
  3. Consider adding a variable for voice gender preference to allow flexibility, even if the default remains male.

Here's a suggested improvement:

- name: Run ovos-config for auto-configuration of STT and TTS based on language
  become: true
  become_user: "{{ ovos_installer_user }}"
  community.docker.docker_container_exec:
    container: ovos_cli
    command: ovos-config autoconfigure --lang {{ ovos_installer_locale }} {{ '--male' if ovos_installer_voice_gender|default('male') == 'male' else '--female' }}
  register: ovos_config_result
  when: ovos_installer_profile == "satellite"
  failed_when: ovos_config_result.rc != 0
  changed_when: ovos_config_result.rc == 0

- name: Debug ovos-config output
  ansible.builtin.debug:
    var: ovos_config_result
  when: ovos_installer_profile == "satellite"

This version adds conditional execution, error handling, and allows for voice gender preference while maintaining the male default.

🛠️ Refactor suggestion

Consider grouping related configuration tasks

The placement of the new auto-configuration task is logical in the current workflow. However, to improve the playbook's organization and readability, consider grouping related configuration tasks together.

You could create a block for all configuration-related tasks, including the Generate _identity.json task and this new auto-configuration task. This would make the playbook structure more clear and easier to maintain. For example:

- name: Configuration tasks
  block:
    - name: Generate _identity.json
      # ... existing task content ...

    - name: Run ovos-config for auto-configuration of STT and TTS based on language
      # ... new task content with suggested improvements ...

  when: ovos_installer_profile == "satellite"

This structure groups related tasks and applies the condition to both tasks, reducing redundancy and improving clarity.

- name: Block uninstall composer
when: ovos_installer_cleaning | bool
tags:
Expand Down
14 changes: 14 additions & 0 deletions ansible/roles/ovos_installer/tasks/virtualenv/venv.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,20 @@
changed_when: false
when: ovos_installer_profile == "satellite"

- name: Run ovos-config for auto-configuration of STT and TTS based on language
become: true
become_user: "{{ ovos_installer_user }}"
ansible.builtin.shell:
cmd: |
ovos-config autoconfigure \
--lang {{ ovos_installer_locale }} \
--male
executable: /bin/bash
environment:
PATH: "{{ ovos_installer_user_home }}/.venvs/ovos/bin"
VIRTUAL_ENV: "{{ ovos_installer_user_home }}/.venvs/ovos"
changed_when: false

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance the auto-configuration task for better flexibility and robustness.

The new task for auto-configuration of STT and TTS is a good addition, but there are a few areas for improvement:

  1. The --male option is hardcoded, which doesn't align with the discussion in the PR comments about allowing flexibility in voice persona selection. Consider making this configurable.

  2. The task lacks a conditional statement, meaning it will run for all installation profiles. This may not be necessary or desirable for all setups.

  3. There's no error handling or reporting for potential failures of the ovos-config command.

Consider applying the following improvements:

  1. Make the voice gender configurable:
ovos-config autoconfigure \
  --lang {{ ovos_installer_locale }} \
  {{ '--male' if ovos_installer_voice_gender == 'male' else '--female' }}
  1. Add a conditional statement to run only for relevant profiles:
when: ovos_installer_profile != 'server'
  1. Add error handling and reporting:
register: ovos_config_result
failed_when: ovos_config_result.rc != 0
  1. Consider adding a changed_when condition based on the output of the command to accurately reflect when changes occur.

These changes will make the task more flexible, efficient, and robust.

- name: Remove {{ ovos_installer_user_home }}/.venvs/ovos Python virtualenv and requirements.txt files
ansible.builtin.file:
path: "{{ ovos_installer_user_home }}/.venvs/ovos"
Expand Down
21 changes: 0 additions & 21 deletions ansible/roles/ovos_installer/templates/mycroft.conf.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,26 +64,5 @@
}
}
{% endif %}
}{% if ovos_installer_profile != "server" %},
"tts": {
"ovos-tts-plugin-server": {
"sentence_tokenize": true,
{% if ovos_installer_locale == "de-de" %}
"voice": "thorsten-low"
{% elif ovos_installer_locale == "en-us" %}
"voice": "ryan-low"
{% elif ovos_installer_locale == "es-es" %}
"voice": "davefx-medium"
{% elif ovos_installer_locale == "fr-fr" %}
"voice": "siwis-medium"
{% elif ovos_installer_locale == "it-it" %}
"voice": "riccardo_fasol-x-low"
{% elif ovos_installer_locale == "nl-nl" %}
"voice": "rdh-medium"
{% elif ovos_installer_locale == "pt-pt" %}
"voice": "tugao-medium"
{% endif %}
}
}
{% endif %}
}
Loading