Switch from the deprecated imp to importlib #2327

forslund · 2019-09-26T11:39:24Z

Description

Replaces the current imp based skill loader with an importlib based solution. This also adds a small change allowing better use of submodules. Before submodules were never reloaded forcing the over-use of the __init__.py file in skills. The change makes the submodules reload together with the main module by removing the refs from sys.modules, allowing better structure of skills.

How to test

Make sure skills load. Make a change to a skill and ensure that the skill is reloaded and that the change is accessible.
Add a log statement to the Spotify skill's spotify.py and check that it's included when the skill reloads.

Contributor license agreement signed?

CLA [ Yes ]

chrisveilleux · 2019-09-26T15:42:18Z

If we are going to do this, I want it to include the ability to load the code from a file other than __init__.py. Putting all your code in this file is a Python anti-pattern. This PR should make it possible for an author to move their skill code from __init__.py to something like skill.py. At the very least, it should make it possible for __init__.py to be able to import code from another file, like skill.py and then contain only the import and the create_skill function.

forslund · 2019-09-26T15:47:45Z

Isn't that already possible?

The only thing that's needed is that __init__.py has the create_skill() function, either locally or imported from a module...

chrisveilleux · 2019-09-26T15:50:23Z

I tried moving the skill class to a file called skill.py a while ago and importing that into __init__.py. it did not work. It is possible that I did it wrong. It is also possible that the new version of importlib supports this whereas the older imp library did not.

I can test against the code in this PR and see if there is different behavior.

forslund · 2019-09-26T15:50:28Z

Ah, right imported submodules might not be reloaded...let me do some tests here

forslund · 2019-09-26T15:53:00Z

Well the spotify skill has the majority of the spotify logic in a separate file so it has worked before at least...

let me try with your case with the entire skill in a skill.py file...

chrisveilleux · 2019-09-26T15:53:49Z

I would be happy if we just didn't hardcode the __init__.py as the only place skill code can come from. Make it so that the code can be loaded from there or from another location, like skill.py. We can support both for a time then remove support for __init__.py.

forslund · 2019-09-26T15:56:42Z

Do you mean like search all files for create_skill()?

Looking at a skill as a module it would require the __init__.py to show what's exported right?

chrisveilleux · 2019-09-26T16:04:44Z

using __init__.py as a way to expose the API of a package is not a Python requirement. Lots of folks just leave that file completely empty. It is my preference the __init__.py be used in all of our code in this way. So if we can get that to work with importlib, that would be ideal. Ideally, we could import create_skill() from skill.py, load it in the skill loader and execute it. I just don't know off the top of my head what is possible and what is not with importlib. If we need to do something a bit different to support how we load skills the way we do, I am OK with that.

chrisveilleux · 2019-09-26T16:05:08Z

OOPS. Hit the close button accidentally.

forslund · 2019-09-26T16:11:33Z

Gotcha.

In any case you're query highlighted an issue here. with this implementation relative imports no longer works in skills so I guess I need to look into that at least.

Will look at loading the skill from any file while I'm at it.

JarbasAl · 2019-09-26T21:35:25Z

Gotcha.

In any case you're query highlighted an issue here. with this implementation relative imports no longer works in skills so I guess I need to look into that at least.

Will look at loading the skill from any file while I'm at it.

relative imports haven't worked ever, i remember in the old times the usual suggestion in slack was to add the skill directory to the python path, i usually started from MYCROFT_ROOT_PATH (remember when skill where part of core?) but i remember seeing all sorts of ugly stuff with os.path and some approaches with skill variables

chrisveilleux · 2019-09-26T22:08:27Z

In case there was any question, manipulating the Python path should be avoided at all costs.

ChanceNCounter · 2019-09-27T02:25:07Z

relative imports haven't worked ever, i remember in the old times the usual suggestion in slack was to add the skill directory to the python path, i usually started from MYCROFT_ROOT_PATH (remember when skill where part of core?) but i remember seeing all sorts of ugly stuff with os.path and some approaches with skill variables

I'm using relative imports in a skill I'm developing right now, and it seems to be working. Aside from this PR, is that going to break later? There seem to be conflicting experiences here.

forslund · 2019-09-27T06:10:34Z

With the current imp implentation it works. I did some work last night and managed to get the relative imports working with importlib as well. It would be terrible if the skills were limited to a single file.

There is an issue with the relative imports not reloading when the __init__.py is reloaded (both in old imp and importlib implementations) but I think I've got a handle on how to tackle that issue.

forslund · 2019-09-27T11:34:50Z

Pushed a WIP update supporting relative imports again and adding reloading for them as well...

Separate python 3.4 and 3.5+ implementation

forslund · 2020-06-29T20:13:14Z

This has been updated to use importlib and a nice and easy fix to allow skill submodules to be reloaded.

devops-mycroft · 2020-06-30T05:18:00Z

Voight Kampff Integration Test Succeeded (Results)

krisgesling

Working great!

One tiny wording nitpick (probably just a personal preference thing) and one clarification.

krisgesling · 2020-06-30T10:33:39Z

mycroft/skills/skill_loader.py

@@ -29,6 +34,49 @@
 SKILL_MAIN_MODULE = '__init__.py'


+def remove_submodule_refs(module_name):
+ """Ensure submodules being reloaded by removing the refs from sys.modules.


#nitpick...

Ensure submodule is reloaded by removing the refs from sys.modules.

Are you ok with

Ensure submodules are reloaded by removing the refs from sys.modules.

Yep that reads great.

Comment updated

krisgesling · 2020-06-30T10:33:56Z

mycroft/skills/skill_loader.py

+ module_name: name of skill module.
+ """
+ submodules = []
+ LOG.info('Skill module'.format(module_name))


Is this intended to be an info level log message?

You're right that should be a debug level, will fix on my train ride home

Changed to debug

devops-mycroft · 2020-06-30T16:19:29Z

Voight Kampff Integration Test Succeeded (Results)

Submodule reload is enforced by removing sys.modules reference. This will make imports do the full uncached importing

devops-mycroft · 2020-06-30T18:36:14Z

Voight Kampff Integration Test Failed (Results)

devops-mycroft · 2020-06-30T20:17:35Z

Voight Kampff Integration Test Failed (Results)

devops-mycroft · 2020-07-01T02:01:14Z

Voight Kampff Integration Test Succeeded (Results)

forslund requested a review from chrisveilleux September 26, 2019 11:39

devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Sep 26, 2019

chrisveilleux closed this Sep 26, 2019

chrisveilleux reopened this Sep 26, 2019

LouPerry approved these changes Nov 1, 2019

View reviewed changes

forslund added the Status: Work in progress PR being actively worked on, not yet ready for review. label Feb 27, 2020

Switch from the deprecated imp to importlib

b968b06

Separate python 3.4 and 3.5+ implementation

forslund force-pushed the feature/importlib branch from 902de98 to a8b1888 Compare June 29, 2020 20:09

forslund added type: feature and removed Status: Work in progress PR being actively worked on, not yet ready for review. labels Jun 29, 2020

MycroftAI deleted a comment from devops-mycroft Jun 30, 2020

krisgesling approved these changes Jun 30, 2020

View reviewed changes

forslund force-pushed the feature/importlib branch from a8b1888 to 3e5e2b4 Compare June 30, 2020 15:27

Handle reloading of relative imports

58608e6

Submodule reload is enforced by removing sys.modules reference. This will make imports do the full uncached importing

forslund force-pushed the feature/importlib branch from 3e5e2b4 to 58608e6 Compare June 30, 2020 17:43

krisgesling merged commit aca6a18 into dev Jul 1, 2020

forslund deleted the feature/importlib branch September 6, 2020 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from the deprecated imp to importlib #2327

Switch from the deprecated imp to importlib #2327

forslund commented Sep 26, 2019 •

edited

Loading

chrisveilleux commented Sep 26, 2019 •

edited

Loading

forslund commented Sep 26, 2019

chrisveilleux commented Sep 26, 2019 •

edited

Loading

forslund commented Sep 26, 2019

forslund commented Sep 26, 2019

chrisveilleux commented Sep 26, 2019 •

edited

Loading

forslund commented Sep 26, 2019

chrisveilleux commented Sep 26, 2019

chrisveilleux commented Sep 26, 2019 •

edited

Loading

forslund commented Sep 26, 2019

JarbasAl commented Sep 26, 2019 •

edited

Loading

chrisveilleux commented Sep 26, 2019

ChanceNCounter commented Sep 27, 2019

forslund commented Sep 27, 2019

forslund commented Sep 27, 2019

forslund commented Jun 29, 2020

devops-mycroft commented Jun 30, 2020

krisgesling left a comment

krisgesling Jun 30, 2020

forslund Jun 30, 2020

krisgesling Jun 30, 2020

forslund Jun 30, 2020

krisgesling Jun 30, 2020

forslund Jun 30, 2020

forslund Jun 30, 2020

devops-mycroft commented Jun 30, 2020

devops-mycroft commented Jun 30, 2020

devops-mycroft commented Jun 30, 2020

devops-mycroft commented Jul 1, 2020

Switch from the deprecated imp to importlib #2327

Switch from the deprecated imp to importlib #2327

Conversation

forslund commented Sep 26, 2019 • edited Loading

Description

How to test

Contributor license agreement signed?

chrisveilleux commented Sep 26, 2019 • edited Loading

forslund commented Sep 26, 2019

chrisveilleux commented Sep 26, 2019 • edited Loading

forslund commented Sep 26, 2019

forslund commented Sep 26, 2019

chrisveilleux commented Sep 26, 2019 • edited Loading

forslund commented Sep 26, 2019

chrisveilleux commented Sep 26, 2019

chrisveilleux commented Sep 26, 2019 • edited Loading

forslund commented Sep 26, 2019

JarbasAl commented Sep 26, 2019 • edited Loading

chrisveilleux commented Sep 26, 2019

ChanceNCounter commented Sep 27, 2019

forslund commented Sep 27, 2019

forslund commented Sep 27, 2019

forslund commented Jun 29, 2020

devops-mycroft commented Jun 30, 2020

krisgesling left a comment

Choose a reason for hiding this comment

krisgesling Jun 30, 2020

Choose a reason for hiding this comment

forslund Jun 30, 2020

Choose a reason for hiding this comment

krisgesling Jun 30, 2020

Choose a reason for hiding this comment

forslund Jun 30, 2020

Choose a reason for hiding this comment

krisgesling Jun 30, 2020

Choose a reason for hiding this comment

forslund Jun 30, 2020

Choose a reason for hiding this comment

forslund Jun 30, 2020

Choose a reason for hiding this comment

devops-mycroft commented Jun 30, 2020

devops-mycroft commented Jun 30, 2020

devops-mycroft commented Jun 30, 2020

devops-mycroft commented Jul 1, 2020

forslund commented Sep 26, 2019 •

edited

Loading

chrisveilleux commented Sep 26, 2019 •

edited

Loading

chrisveilleux commented Sep 26, 2019 •

edited

Loading

chrisveilleux commented Sep 26, 2019 •

edited

Loading

chrisveilleux commented Sep 26, 2019 •

edited

Loading

JarbasAl commented Sep 26, 2019 •

edited

Loading