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.

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

Issue-3100 - Adding skills methods for padatious #3101

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

zeronounours
Copy link

Description

Add two new skills methods to be able to load any file as padatious
intent or entity, even outside of vocab and locale directories.

Fix #3100

How to test

I also added unittests on test_mycroft_skill.py

Contributor license agreement signed?

CLA [x] (Whether you have signed a CLA - Contributor Licensing Agreement

@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Apr 25, 2022
@devops-mycroft
Copy link

devops-mycroft commented Apr 25, 2022

Voight Kampff Integration Test Succeeded (Results)

@krisgesling
Copy link
Contributor

krisgesling commented Apr 26, 2022

Hey there, thanks for helping to improve Mycroft!

I can definitely see the appeal here. I'm tossing up in my mind whether a separate method is necessary or if we should check to see if the intent_file passed into the existing method is a full path?

  • In one sense there is simplicity in having a single method to register intents and a single method to register entities.
  • In another sense it adds complexity to that method since it can now accept more variations.
  • Creating the new method also reduces chances of unexpected behaviours being introduced for existing Skills.

Interested in any opinions on this.

If we do go with a separate method, I'm wondering if we can find a more descriptive name. If I'm developing a Skill and see:

  • register_intent_file()
  • register_intent_file_as()

It's not immediately clear to me how they differ. I might also interpret the new method as if I'm going to register an intent file as something else. Would something like register_intent_file_path() provide a better distinction?

Edit: Just fyi I doubt the VK test failure reported above is related to this PR.

@zeronounours
Copy link
Author

zeronounours commented Apr 28, 2022

The current methods allows the use of full path because of find_resource relying on os.path.join which allows full path components. However, the name of the intent is based on that path and is not sanitized.

Home Assistant skill recently started using full path, but it fails in the end (see MycroftAI/skill-homeassistant#104) as padatious fails to save the network file:

FANN Error 2: Unable to open configuration file "/opt/mycroft/.local/share/mycroft/intent_cache/skill-homeassistant.mycroftai:{/tmp/mycroft/cache/HomeAssistantSkill/tracker}.intent.net" for writing.

Another solution may be to sanitize the path when generating the name to only keep the basename. However, I think some developpers will have trouble understanding why they fails to load my_entity.entity and /tmp/path/my_entity.entity.

The name register_intent_file_path() is fine for me too.

Add two new skills methods to be able to load any file as padatious
intent or entity.

==== Fixed Issues ====
Fix MycroftAI#3100

====  Tech Notes ====
NONE

====  Documentation Notes ====
NONE

==== Localization Notes ====
NONE

==== Environment Notes ====
NONE

==== Protocol Notes ====
Add two new methods to skills to register padatious intents or entities
even outside of `vocab` and `locale` directories
@zeronounours
Copy link
Author

I force-pushed a new version with the proposed names register_intent_file_path and register_entity_file_path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
No open projects
Status: Under active review
Development

Successfully merging this pull request may close these issues.

Allows skill to dynamically define intents or entities
3 participants