Fix wrong import logic when loading modules #140

blackthunder0812 · 2020-01-21T08:56:38Z

Only load module within package, not all accessible modules

Loading all accessible modules causes a bug when an accessible module has a same name with module inside package

Proposed changes:

Only load module within package, not all accessible modules

Status (please check what you already did):

made PR ready for code review
added some tests for the functionality
updated the documentation
updated the changelog
reformat files using black (please check Readme for instructions)

Only load module within package, not all accessible modules Loading all accessible modules causes a bug when an accessible module has a same name with module inside package

wochinge · 2020-01-22T09:25:58Z

@blackthunder0812 Thanks for your contribution! 🎉 Could you please elaborate a bit on the bug so that I can reproduce it?

blackthunder0812 · 2020-01-24T19:38:46Z

@wochinge For example, if we have a project structure like this:

$ tree
.
├── actions
│   └── utils
│       └── module1.py
└── utils
    └── module2.py

Then action server cannot load the module inside actions/utils folder because it already loads modules in utils forder, sample error log:

Nov 29, 2019 @ 15:04:10.2492019-11-29 15:04:10 ERROR    rasa_sdk.executor  - Failed to register package 'actions'.
Nov 29, 2019 @ 15:04:10.249Traceback (most recent call last):
Nov 29, 2019 @ 15:04:10.249  File "/app/rasa_sdk/executor.py", line 175, in register_package
Nov 29, 2019 @ 15:04:10.249    self._import_submodules(package)
Nov 29, 2019 @ 15:04:10.249  File "/app/rasa_sdk/executor.py", line 167, in _import_submodules
Nov 29, 2019 @ 15:04:10.249    results[full_name] = importlib.import_module(full_name)
Nov 29, 2019 @ 15:04:10.249  File "/usr/local/lib/python3.6/importlib/__init__.py", line 126, in import_module
Nov 29, 2019 @ 15:04:10.249    return _bootstrap._gcd_import(name[level:], package, level)
Nov 29, 2019 @ 15:04:10.249  File "<frozen importlib._bootstrap>", line 994, in _gcd_import
Nov 29, 2019 @ 15:04:10.249  File "<frozen importlib._bootstrap>", line 971, in _find_and_load
Nov 29, 2019 @ 15:04:10.249  File "<frozen importlib._bootstrap>", line 953, in _find_and_load_unlocked
	Nov 29, 2019 @ 15:04:10.249
ModuleNotFoundError: No module named 'actions.utils.chatette_fix'

Reproducing bug with above project structure:

for loader, name, is_pkg in pkgutil.walk_packages(package.__path__):
    full_name = package.__name__ + "." + name
    results[full_name] = importlib.import_module(full_name)

Because we have 2 directories with the same name utils (/utils and /actions/utils), function pkgutil.walk_packages incorrectly yield packages in /utils directory:

>>> package = importlib.import_module('actions')
DEBUG:actions:LOADED CONFIG BOT
>>> pprint.pprint([(loader, name, is_pkg) for loader, name, is_pkg in pkgutil.walk_packages(package.__path__)])
[(FileFinder('/app/actions'), 'default_ask_affirmation', False),
 (FileFinder('/app/actions'), 'fee_ship', False),
 (FileFinder('/app/actions'), 'find_product', False),
 (FileFinder('/app/actions'), 'find_product_by_category', False),
 (FileFinder('/app/actions'), 'graphdb', True),
 (FileFinder('/app/actions'), 'handover_to_inbox', False),
 (FileFinder('/app/actions'), 'order_product', False),
 (FileFinder('/app/actions'), 'order_product_form', False),
 (FileFinder('/app/actions'), 'query_kb', False),
 (FileFinder('/app/actions'), 'response_total_cost', False),
 (FileFinder('/app/actions'), 'search_engine', True),
 (FileFinder('/app/actions'), 'start_conversation', False),
 (FileFinder('/app/actions'), 'utils', True),
 (FileFinder('/app/utils'), 'utils.chatette_fix', False),
 (FileFinder('/app/utils'), 'utils.facebook_utils', False),
 (FileFinder('/app/utils'), 'utils.log_config', False),
 (FileFinder('/app/utils'), 'utils.lookup_tables', False)]

Then we can fix by loading only modules inside packages, recursively, don't load all accessible modules

wochinge

@blackthunder0812

I tried with this structure

.
└── test
    ├── __init__.py
    ├── actions.py . # imports module1 and module2
    ├── other
    │   ├── __init__.py
    │   └── utils
    │       ├── __init__.py
    │       └── module1.py
    └── utils
        ├── __init__.py
        └── module2.py

and running rasa run actions --actions test which works fine. When I use your fix it actually breaks (gettig ModuleNotFoundError: No module named 'test.testactions') .

Which command are you using to run the action server and did you include all the __init__ files to mark the directories as Python packages?

blackthunder0812 · 2020-02-04T08:42:02Z

@blackthunder0812

I tried with this structure
.
└── test
    ├── __init__.py
    ├── actions.py . # imports module1 and module2
    ├── other
    │   ├── __init__.py
    │   └── utils
    │       ├── __init__.py
    │       └── module1.py
    └── utils
        ├── __init__.py
        └── module2.py
and running rasa run actions --actions test which works fine. When I use your fix it actually breaks (gettig ModuleNotFoundError: No module named 'test.testactions') .

Which command are you using to run the action server and did you include all the __init__ files to mark the directories as Python packages?

Thanks for pointing out the problem.

My project structure is different, please look at images below, (actions folder and utils folder are at same level) this is before patch:

This is after new patch:

blackthunder0812 · 2020-02-04T08:56:00Z

It works even if I remove all __init__.py files

blackthunder0812 · 2020-02-13T09:00:46Z

@wochinge Can you please try again?

wochinge · 2020-02-17T08:40:29Z

@blackthunder0812 I tried again, and your change works, but

it also works with the old code for me
seems like it's traversing some things multiple times with your code.

I adapted my structure to yours:

.
├── __init__.py
├── actions.py
├── other
│   ├── __init__.py
│   └── utils
│       ├── __init__.py
│       └── module1.py
└── utils
    ├── __init__.py
    └── module2.py

And then run rasa run actions --actions actions`` where actions.py` imports

from utils.module2 import MODULE_2
from other.utils.module1 import MODULE_1

Works like a charm

blackthunder0812 · 2020-02-17T10:57:04Z

@wochinge

The problem does not lie on importing module in actions.py file, but in importing modules from loading modules in action. To reproduce problem, construct a project structure as following:

.
├── actions
│   ├── actions.py
│   └── utils
│       ├── __init__.py
│       └── module2.py
├── __init__.py
└── utils
    ├── __init__.py
    └── module1.py

Then run:

rasa run actions --actions actions

with actions.py is empty.

Before patching:

2020-02-17 17:51:23 INFO     rasa_sdk.endpoint  - Starting action endpoint server...
2020-02-17 17:51:23 ERROR    rasa_sdk.executor  - Failed to register package 'actions'.
Traceback (most recent call last):
  File "/home/sonbn/Workspace/venvs/rasa-dev/lib/python3.6/site-packages/rasa_sdk/executor.py", line 206, in register_package
    self._import_submodules(package)
  File "/home/sonbn/Workspace/venvs/rasa-dev/lib/python3.6/site-packages/rasa_sdk/executor.py", line 198, in _import_submodules
    results[full_name] = importlib.import_module(full_name)
  File "/usr/lib/python3.6/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 994, in _gcd_import
  File "<frozen importlib._bootstrap>", line 971, in _find_and_load
  File "<frozen importlib._bootstrap>", line 953, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'actions.utils.module1'

Please look at screenshot below

The difference between ours is that my --actions flag point to actions folder, which is a package, not actions.py file

wochinge · 2020-03-05T10:33:40Z

The problem does not lie on importing module in actions.py file, but in importing modules from loading modules in action. To reproduce problem, construct a project structure as following:

In my opinion the error in your current directory structure makes sense. You are importing the actions module which references a package which is not in your PYTHONPATH. In my opinion utils (the one including module 1) should be part of your actions module or you have to add it to your PYTHONPATH.

blackthunder0812 · 2020-03-05T10:52:40Z

But I don't actually import anything, please note that all files are empty. Then the problem is that even if I don't import any module at outer utils folder, action server will still crash.

The project structure as above is quite popular and should be valid I think.

wochinge · 2020-04-21T12:00:48Z

But I don't actually import anything, please note that all files are empty.

Even if the files are empty, you're still importing a module (even if it's empty), no?

Then the problem is that even if I don't import any module at outer utils folder, action server will still crash.

Can you maybe upload zip for me so that I can reproduce it? I think that would make it easier 🙏

blackthunder0812 · 2020-05-07T04:59:04Z

@wochinge Please check, test command:

python -m rasa_sdk --actions actions -v -vv

actions.zip

wochinge · 2020-05-19T08:23:55Z

Sorry, for the super long delay @blackthunder0812 . Finally I could re-produce the problem 👍 🙏

Two things:

your solution seems to add a couple of round trips to the importing. _import_submodules seems to be called repeatedly on the same thing. I tried a different solution which simply skips loading which are modules above the directory of the current package. Please let me know if you can see any downsides with this approach.

for loader, name, is_pkg in pkgutil.walk_packages(package.__path__):
    if loader.path not in package.__path__:
        continue
   
   # rest of the code

Could you please add a test for the fix? E.g. re-recreating the file structure which you sent in the actions.zip and then trying to import the submodules from it

Thanks again for being so patient!

CLAassistant · 2020-06-12T08:46:51Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

blackthunder0812 requested a review from wochinge January 21, 2020 09:03

Fix wrong import logic when loading modules

13b1d69

Only load module within package, not all accessible modules Loading all accessible modules causes a bug when an accessible module has a same name with module inside package

blackthunder0812 force-pushed the patch-1 branch from 1aa109d to 13b1d69 Compare January 22, 2020 02:57

blackthunder0812 requested review from ricwo and tmbo January 22, 2020 03:00

tmbo removed request for tmbo and ricwo January 22, 2020 16:57

wochinge reviewed Jan 30, 2020

View reviewed changes

blackthunder0812 force-pushed the patch-1 branch from ff8200f to 587ab88 Compare February 4, 2020 08:44

Fix breaking import

47603fe

blackthunder0812 force-pushed the patch-1 branch from 587ab88 to 47603fe Compare February 4, 2020 08:44

erohmensing mentioned this pull request Mar 4, 2020

Inconsistency in initial project setup for actions running via rasa or via sdk RasaHQ/rasa#5368

Closed

wochinge closed this Nov 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wrong import logic when loading modules #140

Fix wrong import logic when loading modules #140

blackthunder0812 commented Jan 21, 2020

wochinge commented Jan 22, 2020

blackthunder0812 commented Jan 24, 2020 •

edited

Loading

wochinge left a comment •

edited

Loading

blackthunder0812 commented Feb 4, 2020

blackthunder0812 commented Feb 4, 2020

blackthunder0812 commented Feb 13, 2020

wochinge commented Feb 17, 2020

blackthunder0812 commented Feb 17, 2020 •

edited

Loading

wochinge commented Mar 5, 2020 •

edited

Loading

blackthunder0812 commented Mar 5, 2020

wochinge commented Apr 21, 2020

blackthunder0812 commented May 7, 2020

wochinge commented May 19, 2020 •

edited

Loading

CLAassistant commented Jun 12, 2020

Fix wrong import logic when loading modules #140

Fix wrong import logic when loading modules #140

Conversation

blackthunder0812 commented Jan 21, 2020

wochinge commented Jan 22, 2020

blackthunder0812 commented Jan 24, 2020 • edited Loading

wochinge left a comment • edited Loading

Choose a reason for hiding this comment

blackthunder0812 commented Feb 4, 2020

blackthunder0812 commented Feb 4, 2020

blackthunder0812 commented Feb 13, 2020

wochinge commented Feb 17, 2020

blackthunder0812 commented Feb 17, 2020 • edited Loading

wochinge commented Mar 5, 2020 • edited Loading

blackthunder0812 commented Mar 5, 2020

wochinge commented Apr 21, 2020

blackthunder0812 commented May 7, 2020

wochinge commented May 19, 2020 • edited Loading

CLAassistant commented Jun 12, 2020

blackthunder0812 commented Jan 24, 2020 •

edited

Loading

wochinge left a comment •

edited

Loading

blackthunder0812 commented Feb 17, 2020 •

edited

Loading

wochinge commented Mar 5, 2020 •

edited

Loading

wochinge commented May 19, 2020 •

edited

Loading