Improved startup time #8141

tmbo · 2021-03-08T16:59:44Z

Proposed changes:

avoid importing tensorflow where possible in CLI (tensorflow has an import time of ~4s)
command to profile imports locally (requires https://github.com/nschloe/tuna to be installed and python >= 3.7):
```
python -X importtime -m rasa.__main__ --help  2> import.log
tuna import.log
```
fixes Improve Rasa CLI start-up time #4280
added a test to avoid a regression (checks if tensorflow gets imported on CLI start)

Status (please check what you already did):

added some tests for the functionality
updated the documentation
updated the changelog (please check changelog for instructions)
reformat files using black (please check Readme for instructions)

tmbo · 2021-03-08T17:00:42Z

changes make the commands --help & init a lot quicker, but I don't think it makes sense to merge anything without a change as it is very easy to break this again.

wochinge

Thanks for tackling this! This is both amazing for the users and our CI speed 🚀

Regarding the regression test: How about testing for the tensorflow import instead of a certain timing? E.g. you could write a script which

imports rasa
sets sys.argv according to the commands you want to test
runs the command (maybe with mocked behavior)
assert that tensorflow isn't in the imported modules

rasa/cli/data.py

rasa/telemetry.py

rasa/run.py

rasa/__init__.py

tmbo · 2021-03-10T20:39:36Z

ready for another review 😉

wochinge

Thanks for wrapping this up so quickly! Let a few more minor comments but good to go from my side!

rasa/api.py

rasa/core/config.py

rasa/shared/core/constants.py

rasa/core/config.py

tests/cli/test_cli.py

tests/shared/core/test_constants.py

wochinge · 2021-03-11T09:25:51Z

@tmbo How severe is the problem with clashing imports? Would be nice to get it in the release today 👀

Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com>

tmbo · 2021-03-11T10:25:23Z

@wochinge I made the other changes, but I can't look into the import issue today, my suspicion is that it worked beforehand because had

from rasa.train import train

in rasa.__init__ which populated the global state FIRST with rasa.train (module) and AFTERWARDS with rasa.train() (function) - due to the evaluation order.

Now, we are not importing rasa.train (module) in init anymore, which means that as soon as we do that for the first time, it apparently overwrites the defined function (and apparently windows is using a different resolution logic here?)

state on master:

state on this branch:

tmbo · 2021-03-11T10:26:37Z

If you have time to investigate or try moving this around I am happy to help but I won't have enough time today to dig deeper myself 😞

wochinge · 2021-03-11T10:30:21Z

Thanks for the heads up. I'll have a short look but it doesn't seem like something we should rush just to get it in the release. Especially as it might require change to our Python API.

tmbo · 2021-03-11T10:39:55Z

I mean there is one quick solution we could try: move the functions back into the original modules and move all the imports in there into the functions (or at least all rasa ones). That will likely work and we could revisit this later.

wochinge · 2021-03-11T13:06:21Z

I mean there is one quick solution we could try: move the functions back into the original modules and move all the imports in there into the functions (or at least all rasa ones). That will likely work and we could revisit this later.

This seems way harder to maintain. I'm in favor of having these things explict. In my opinion it would be fine to rename the modules (e.g. train -> model_training, test -> model_testing and create_agent from run can go to server.py). They are not part of the official Python API as the official Python API is to use rasa.train(...) etc. so in my opinion this would be ok to do, what do you think?

tmbo · 2021-03-11T14:33:22Z

well, if they don't appear in the docs I am good with that 👍

wochinge · 2021-03-11T15:55:11Z

I searched

import rasa.core
import rasa.nlu
import rasa.test
from rasa import
from rasa.core (has matches but not related to train, run, test
from rasa.nlu
from rasa.test

and had 0 matches 🎉 So good to rename 🚀

tmbo · 2021-03-15T13:21:21Z

yes that makes sense 👍

tmbo · 2021-03-15T13:22:36Z

@wochinge are you able to make these changes? Otherwise I'll put it in enable's inbox to make these additional changes.

wochinge · 2021-03-15T15:23:10Z

Sure, will wrap it up later 👍🏻

wochinge · 2021-03-15T17:08:03Z

What about deleted the __init__ imports (here and here) instead of renaming the train / visualize in rasa.core and train / test in rasa.nlu?

They aren't used except by us and in my opinion users should rather go through the __init__ imports in rasa or rasa.model_training, what do you think? This also avoids needless imports this stuff.

…nlu.cross_validate`

wochinge · 2021-03-16T09:24:29Z

rasa/core/agent.py

@@ -269,6 +270,33 @@ async def schedule_model_pulling(
    )


+def create_agent(model: Text, endpoints: Text = None) -> "Agent":


moved from rasa.run

wochinge · 2021-03-16T09:26:11Z

@tmbo Changes are ready. Do you have time for a short review? 🙌🏻

tmbo

@wochinge can't approve since I opened the PR - but changes look good 👍

rasa/model_training.py

rasa/model_testing.py

improved startup time

b294aa5

tmbo requested a review from wochinge March 8, 2021 16:59

wochinge reviewed Mar 10, 2021

View reviewed changes

rasa/cli/data.py Outdated Show resolved Hide resolved

rasa/telemetry.py Show resolved Hide resolved

rasa/run.py Outdated Show resolved Hide resolved

rasa/__init__.py Outdated Show resolved Hide resolved

wochinge mentioned this pull request Mar 10, 2021

Investigate test_rasa_data: All conversions take 10+ seconds #8128

Closed

tmbo added 2 commits March 10, 2021 19:47

merged main

081de27

use policy name constants instead of importing classes

7b9def9

tmbo requested a review from a team as a code owner March 10, 2021 19:31

tmbo added 2 commits March 10, 2021 20:38

updated telemetry example

1121de9

added cli import test

0a9c14c

tmbo changed the title ~~WIP improved startup time~~ Improved startup time Mar 10, 2021

added changelog

61cbcd7

tmbo requested a review from wochinge March 10, 2021 20:39

tmbo added 2 commits March 10, 2021 21:53

improved docstrings

ce29b4d

fixed data test

1f8a27f

wochinge approved these changes Mar 11, 2021

View reviewed changes

tmbo and others added 4 commits March 11, 2021 10:56

Update tests/shared/core/test_constants.py

6752dc1

Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com>

Update rasa/api.py

d7446f4

Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com>

added fallback classifier name constant

ea8dc62

fixed not defined name

2272768

use Python from sys.executable to get poetry's python version

8afca53

wochinge mentioned this pull request Mar 15, 2021

Switch to mypy part1 #6511

Merged

12 tasks

wochinge self-assigned this Mar 15, 2021

wochinge added 8 commits March 15, 2021 17:41

rename rasa.train [production code]

f1e4f4b

rename rasa.train [test code]

272eba9

rename rasa.test [production code]

67968df

rename rasa.test [test code]

dae1fff

move rasa.run.create_agent to rasa.core.agent

6ee8865

rename rasa.run [production code]

dc88cb0

adapt mocking in test

a218e86

Merge branch 'main' into improved-startup-time

3e6340b

wochinge added 6 commits March 15, 2021 19:03

remove shorthand import for rasa.core.train and rasa.core.visualize

a545326

remove shorthand syntax for rasa.nlu.train, rasa.nlu.test, `rasa.…

3c9a265

…nlu.cross_validate`

add changelogs

180291b

add missing docstrings

153ec8d

fixes for prior module renames / removing of abbreviated imports

c9db254

Merge branch 'main' into improved-startup-time

13c6b39

wochinge reviewed Mar 16, 2021

View reviewed changes

tmbo commented Mar 16, 2021

View reviewed changes

rasa/model_training.py Show resolved Hide resolved

rasa/model_testing.py Show resolved Hide resolved

Merge branch 'main' into improved-startup-time

830e331

wochinge enabled auto-merge March 16, 2021 11:15

wochinge merged commit 2dd3fe9 into main Mar 16, 2021

wochinge deleted the improved-startup-time branch March 16, 2021 11:19

wochinge mentioned this pull request Apr 1, 2021

AttributeError: module 'tensorflow_core._api.v2.config' has no attribute 'list_physical_devices' #8022

Closed

joejuzl mentioned this pull request Apr 8, 2021

Some CLI commands are unexpectedly slow #8058

Closed

m-vdb mentioned this pull request Jul 23, 2021

Remove outdated backwards compat code #9187

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved startup time #8141

Improved startup time #8141

tmbo commented Mar 8, 2021 •

edited

tmbo commented Mar 8, 2021

wochinge left a comment

tmbo commented Mar 10, 2021

wochinge left a comment

wochinge commented Mar 11, 2021

tmbo commented Mar 11, 2021

tmbo commented Mar 11, 2021

wochinge commented Mar 11, 2021

tmbo commented Mar 11, 2021

wochinge commented Mar 11, 2021

tmbo commented Mar 11, 2021

wochinge commented Mar 11, 2021

tmbo commented Mar 15, 2021

tmbo commented Mar 15, 2021

wochinge commented Mar 15, 2021

wochinge commented Mar 15, 2021

wochinge Mar 16, 2021

wochinge commented Mar 16, 2021

tmbo left a comment •

edited

		@@ -269,6 +270,33 @@ async def schedule_model_pulling(
		)


		def create_agent(model: Text, endpoints: Text = None) -> "Agent":

Improved startup time #8141

Improved startup time #8141

Conversation

tmbo commented Mar 8, 2021 • edited

tmbo commented Mar 8, 2021

wochinge left a comment

Choose a reason for hiding this comment

tmbo commented Mar 10, 2021

wochinge left a comment

Choose a reason for hiding this comment

wochinge commented Mar 11, 2021

tmbo commented Mar 11, 2021

tmbo commented Mar 11, 2021

wochinge commented Mar 11, 2021

tmbo commented Mar 11, 2021

wochinge commented Mar 11, 2021

tmbo commented Mar 11, 2021

wochinge commented Mar 11, 2021

tmbo commented Mar 15, 2021

tmbo commented Mar 15, 2021

wochinge commented Mar 15, 2021

wochinge commented Mar 15, 2021

wochinge Mar 16, 2021

Choose a reason for hiding this comment

wochinge commented Mar 16, 2021

tmbo left a comment • edited

Choose a reason for hiding this comment

tmbo commented Mar 8, 2021 •

edited

tmbo left a comment •

edited