Add autoconfig support to installer #170
Conversation
|
@goldyfruit will this file structure work, I will ask user for autoconfig questions and then run the command accordingly, to tackle the region issue we can find region using their ip to detect their country so if someone chooses English in Australia then its en-au, |
i wouldnt worry about this for now, as the translations currently are all for en-us , setting a different code might just cause issues with no benefit only portuguese currently differentiates between pt-br and pt-pt, no other language supports different dialect lang codes (yet) |
|
I would not prompt any questions about the auto configuration. By default if French is selected then the assistant should be configured to in French, etc... About the male or female voice, this question should be handle by the core team as its part of the OVOS's persona. @JarbasAl @j1nx Do we want to have a male or female persona? |
goldyfruit
added
enhancement
This reverts commit 5a7afae.
If not prompting user, keep it male |
I guess the standard for voice assistant is female ? |
historically we have used the male voice from alan pope, which goes well with "hey mycroft". let's keep it that way besides, its a good differentiation factor in comparison with other voice assistants |
|
@suvanbanerjee I just pushed a commit, please have a look. |
WalkthroughThe pull request introduces new tasks in Ansible playbooks for auto-configuration of Speech-to-Text (STT) and Text-to-Speech (TTS) using the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
ansible/roles/ovos_installer/tasks/docker/composer.yml (1)
Missing IP detection implementation and incomplete Jinja2 template updates
- IP Detection: The PR does not implement IP detection for region determination as outlined in the objectives.
- Jinja2 Template: The changes to
mycroft.conf.j2solely remove configurations without the expected updates, which contradicts the AI-generated summary.🔗 Analysis chain
Line range hint
1-85: Clarify discrepancies between PR objectives, summary, and implemented changesThere are some inconsistencies between the PR objectives, the AI-generated summary, and the changes implemented in this file:
- The PR objectives mention using IP detection for region determination, but this is not implemented in the current file.
- The AI-generated summary mentions updates to the Jinja2 template for
mycroft.conf.j2, which is not present in this file.Could you please clarify if these features are implemented in other files not provided for review? If so, it would be helpful to see those changes as well to ensure a comprehensive review of the autoconfiguration feature.
To verify the implementation of these features, you can run the following script:
This script will search for any IP detection related code in the Ansible files and show the diff for the
mycroft.conf.j2template if it has been updated in this PR.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for IP detection and Jinja2 template updates # Check for IP detection implementation echo "Checking for IP detection implementation:" rg --type yaml "ip.*detect" ansible/ # Check for updates to mycroft.conf.j2 echo "Checking for updates to mycroft.conf.j2:" git diff HEAD~1 -- ansible/roles/ovos_installer/templates/mycroft.conf.j2Length of output: 1373
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- ansible/roles/ovos_installer/tasks/docker/composer.yml (1 hunks)
- ansible/roles/ovos_installer/tasks/virtualenv/venv.yml (1 hunks)
- ansible/roles/ovos_installer/templates/mycroft.conf.j2 (0 hunks)
💤 Files with no reviewable changes (1)
- ansible/roles/ovos_installer/templates/mycroft.conf.j2
🧰 Additional context used
| - 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 | ||
|
|
There was a problem hiding this comment.
🛠️ 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:
- Add conditional execution based on the
ovos_installer_profileto ensure it only runs for the appropriate setup (e.g., satellite profile). - Implement error handling and output capturing for better troubleshooting capabilities.
- 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: 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 | ||
|
|
There was a problem hiding this comment.
🛠️ 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:
-
The
--maleoption is hardcoded, which doesn't align with the discussion in the PR comments about allowing flexibility in voice persona selection. Consider making this configurable. -
The task lacks a conditional statement, meaning it will run for all installation profiles. This may not be necessary or desirable for all setups.
-
There's no error handling or reporting for potential failures of the
ovos-configcommand.
Consider applying the following improvements:
- Make the voice gender configurable:
ovos-config autoconfigure \
--lang {{ ovos_installer_locale }} \
{{ '--male' if ovos_installer_voice_gender == 'male' else '--female' }}- Add a conditional statement to run only for relevant profiles:
when: ovos_installer_profile != 'server'- Add error handling and reporting:
register: ovos_config_result
failed_when: ovos_config_result.rc != 0- Consider adding a
changed_whencondition based on the output of the command to accurately reflect when changes occur.
These changes will make the task more flexible, efficient, and robust.
|
|
I removed the |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- ansible/roles/ovos_installer/tasks/docker/composer.yml (1 hunks)
- ansible/roles/ovos_installer/tasks/virtualenv/venv.yml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ansible/roles/ovos_installer/tasks/docker/composer.yml
🧰 Additional context used
|
thanks @goldyfruit for the pr's it made my job super easy 😅, i think this pr is ready to review and push, if there are any more changes please require do tell me |
I mostly did it as you can learn from it for the next time. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
ansible/roles/ovos_installer/tasks/virtualenv/venv.yml (1)
121-135: Consider documenting the task order rationaleThe placement of the new auto-configuration task after the
_identity.jsongeneration task seems appropriate. However, it's important to ensure that this order is intentional and necessary for the correct configuration of the system.Consider adding a comment above the task to explain why this specific order is chosen. This will help future maintainers understand the reasoning behind the task placement and prevent potential issues if the order is changed inadvertently.
For example:
# Auto-configuration is performed after identity setup to ensure all necessary # components are in place for language and voice configuration. - name: Run ovos-config for auto-configuration of STT and TTS based on language # ... (rest of the task remains the same)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- ansible/roles/ovos_installer/tasks/docker/composer.yml (1 hunks)
- ansible/roles/ovos_installer/tasks/virtualenv/venv.yml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ansible/roles/ovos_installer/tasks/docker/composer.yml
🧰 Additional context used
|
@suvanbanerjee have you tried this PR? |
Closes #157
Summary by CodeRabbit
New Features
Bug Fixes