Tracker entity fails padatious compilation

[zeronounours]

Describe the bug
Since PR #89 the list of trackers is saved within a temporary tracker.entity files and loaded within padatious. Padatious cannot save the generated network and fails with an exception.

When padatious is configured to be single_thread, it cause the whole skills process to fails and exit.

To Reproduce
Steps to reproduce the behavior:

1. Configure padatious as single_thread
2. Configure HomeAssistantSkill to an instance with trackers
3. Start mycroft.skills
4. See error in logs and the process being aborted

Expected behavior
Padatious should not fails to compile and save the generated neural network.

Log files

2022-04-24 15:58:18.300 | INFO     |  3366 | mycroft.skills.intent_services.padatious_service:train:151 | Training... (single_thread=True)
2022-04-24 15:58:18.343 | INFO     |  3366 | __main__:on_ready:185 | Skills service is ready.
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.
malloc(): invalid size (unsorted)

Environment (please complete the following information):

• Device type: Raspberry Pi
• OS: Raspbian
• Mycroft-core version: 21.2.2

Additional context

The issue comes from method _register_tracker_entities:

skill-homeassistant/__init__.py

Lines 91 to 107 in cb27f22

	def _register_tracker_entities(self) -> None:
	"""List tracker entities.
	Add them to entity file and registry it so
	Padatious react only to known entities.
	Should fix conflict with Where is skill.
	"""
	types = ['device_tracker']
	entities = self.ha_client.list_entities(types)
	if entities:
	cache_dir = get_cache_directory(type(self).__name__)
	self.tracker_file = pth_join(cache_dir, "tracker.entity")
	with open(self.tracker_file, 'w', encoding='utf8') as voc_file:
	voc_file.write('\n'.join(entities))
	self.register_entity_file(self.tracker_file)

This methods create a temporary file (/tmp/mycroft/cache/HomeAssistantSkill/tracker.entity) and write trackers to it. It is provided to the core method register_entity_file which is intented to managed files within the vocab or locale directories only.

When Mycroft core generate the name for padatious, it uses both the skill ID and the filename https://github.com/MycroftAI/mycroft-core/blob/dev/mycroft/skills/mycroft_skill/mycroft_skill.py#L1049

In this case, the entity name contains multiples slashes / which are not valid within filenames.

One solution to prevent this issue would be to directly write list of trackers within vocab/<lang>/tracker.entity and to load it with self.register_entity_file("tracker.entity")

[Tony763]

Hello, when I did this method, one of proposed tracker.entity storage was directly in folder with skill. It was rejected as not a good solution. If the skill work with padatious not single_threated=false, I would raise issue in Padatious.

When I come back home, I will try this.

[zeronounours]

The issue is not really within padatious. It may do some cleanup of names to prevent saving issues, but the core problem remains this skill as it uses an unexpected behavior of mycroft-core: the filename is looked up within the vocab or locale, but it relies on os.path.join:

path = join(self.root_dir, res_dirname, lang, res_name)

Due to res_name being absolute, join only return this part. However it is not an expected behavior, see find_ressource docstring: https://github.com/MycroftAI/mycroft-core/blob/a909fc8f197aeb069999135dfe6ad1579ff69772/mycroft/skills/mycroft_skill/mycroft_skill.py#L773-L800

However, I do agree on the fact temporary files are a better design. It would be interesting for mycroft-core to provide a way to dynamically create entities or intents. I may create a PR going this way on mycroft-core

[Tony763]

Yes, that's something I like what I did mean with Padatious (not in skill, but in Mycroft - wrote on phone in hurry, sry).

I can imagine something like this:
find_resource could search not just root of skill, but also temp directory of skill. Then we could create a same directory tree as with skill root.

or

When res_name is absolute path, the we could just check if file exist.

Or both.

[zeronounours]

A cleaner way to do, even if mycroft-core do not have a dedicated method would be to do what register_entity_file do

             name = '{}:tracker'.format(self.skill_id)
             self.intent_service.register_padatious_entity(name, self.tracker_file)

instead of

             self.register_entity_file(self.tracker_file) 

---

     def _register_tracker_entities(self) -> None: 
         """List tracker entities. 
  
         Add them to entity file and registry it so 
         Padatious react only to known entities. 
         Should fix conflict with Where is skill. 
         """ 
         types = ['device_tracker'] 
         entities = self.ha_client.list_entities(types) 
  
         if entities: 
             cache_dir = get_cache_directory(type(self).__name__) 
             self.tracker_file = pth_join(cache_dir, "tracker.entity") 
  
             with open(self.tracker_file, 'w', encoding='utf8') as voc_file: 
                 voc_file.write('\n'.join(entities)) 
             name = '{}:tracker'.format(self.skill_id)
             self.intent_service.register_padatious_entity(name, self.tracker_file)

[zeronounours]

Do you want me to do a PR, or do you prefer to do it directly as part of another commit because it is only a 2-lines change?

[Tony763]

Please, prepare a separate PR, I am quite out of time now and It's always better to create separate PR's.

I had to cut one big PR into slices myself in order to get it merged. 🙂