Avoid retraining Core model if only templates section is changed #4251

msamogh · 2019-08-15T06:01:13Z

Proposed changes:

Fix The whole Core model is retrained even if only the templates section is changed #4216

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)

tmbo · 2019-09-03T14:28:00Z

@msamogh what is missing to move this PR forward?

msamogh · 2019-09-04T09:06:27Z

tests/core/conftest.py:239: in trained_model
    return train_model(project)
tests/core/conftest.py:232: in train_model
    rasa.train(domain, config, training_files, output)
rasa/train.py:40: in train
    kwargs=kwargs,
uvloop/loop.pyx:1417: in uvloop.loop.Loop.run_until_complete
    ???
rasa/train.py:87: in train_async
    kwargs,
rasa/train.py:170: in _train_async_internal
    kwargs=kwargs,
rasa/train.py:216: in _do_training
    kwargs=kwargs,
rasa/train.py:346: in _train_core_with_validated_data
    kwargs=kwargs,
rasa/core/train.py:71: in train
    agent.persist(output_path, dump_stories, replace_templates_only)
rasa/core/agent.py:799: in persist
    self.domain.persist(os.path.join(model_path, DEFAULT_DOMAIN_PATH))
rasa/core/domain.py:638: in persist
    utils.dump_obj_as_yaml_to_file(filename, domain_data)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

filename = '/var/folders/87/pnn5hwg11cb8qkgp_q4dm8280000gp/T/tmpl_ucro1v/core/domain.yml'
obj = {'actions': ['utter_cheer_up', 'utter_did_that_help', 'utter_goodbye', 'utter_greet', 'utter_happy'], 'config': {'store_entities_as_slots': True}, 'entities': [], 'forms': [], ...}

    def dump_obj_as_yaml_to_file(filename: Union[Text, Path], obj: Dict) -> None:
        """Writes data (python dict) to the filename in yaml repr."""
>       with open(str(filename), "w", encoding="utf-8") as output:
E       FileNotFoundError: [Errno 2] No such file or directory: '/var/folders/87/pnn5hwg11cb8qkgp_q4dm8280000gp/T/tmpl_ucro1v/core/domain.yml'

rasa/core/utils.py:227: FileNotFoundError

I suspect that it could be because a temp directory somewhere is being cleaned up prematurely.

msamogh · 2019-09-04T09:15:52Z

Think I found the cause. Had switched the order of persists.

msamogh · 2019-09-04T14:28:13Z

Travis job link - https://travis-ci.com/RasaHQ/rasa/builds/125875507

msamogh · 2019-09-04T16:31:56Z

@tmbo Looks like good to go

tmbo · 2019-09-04T22:26:35Z

great work ⭐

As far as I can see, can go into a patch. Will release as part of 1.3.1

tmbo

I really like the idea, but I'd like to avoid adding the template flag to the retrain method. I'd rather directly only call the domain to persist from the method that checks the fingerprints

tmbo · 2019-09-30T20:15:54Z

rasa/model.py

            target_path = os.path.join(train_path, "core")
            retrain_core = not merge_model(old_core, target_path)
-
-        if not nlu_fingerprint_changed(last_fingerprint, new_fingerprint):
+        else:


I don't think the logic is correct here, as this else will be run if core needs to be retrained.

I'm not sure if I understood your concern here correctly. Perhaps you can mention a specific combination of values for retrain_core and retrain_nlg that you think this will fail for?

I think Tom means: why do we need to merge core in case retrain_core=True?
In general I don't like that should_retrain already does some modifications. I think that's unexpected behavior when I call a function should_xy_be_done. And I also think that's part of the reason why this seems so complicated (to me 😄 ).

The docstring also indicates a totally different behavior of the function

So the objectives are as follows:

should_retrain.nlg should return the value specific to whether only the NLG section has changed regardless of the value of should_retrain.core. This is to ensure the returned values make logical sense.

Given that its value should be independent of should_retrain.core, we also want to check whether it's possible to merge the directories, because in the following case when Core needn't be retrained and merging directories is failing.

But have updated the condition now to make it cleaner and less confusing.

rasa/train.py

2. Short-circuit flag before reaching the actual train method

outdated

wochinge

The functionality seems great. However, I think the check for the NLG section is the cherry on top of some technical debt we are having here, and we should untangle some of the things first to make it easier understandable / maintainable.

CHANGELOG.rst

rasa/model.py

rasa/train.py

tests/core/test_model.py

wochinge · 2019-10-09T07:49:36Z

rasa/model.py

            target_path = os.path.join(train_path, "core")
            retrain_core = not merge_model(old_core, target_path)
-
-        if not nlu_fingerprint_changed(last_fingerprint, new_fingerprint):
+        else:


I think Tom means: why do we need to merge core in case retrain_core=True?
In general I don't like that should_retrain already does some modifications. I think that's unexpected behavior when I call a function should_xy_be_done. And I also think that's part of the reason why this seems so complicated (to me 😄 ).

wochinge · 2019-10-09T07:51:22Z

rasa/model.py

            target_path = os.path.join(train_path, "core")
            retrain_core = not merge_model(old_core, target_path)
-
-        if not nlu_fingerprint_changed(last_fingerprint, new_fingerprint):
+        else:


The docstring also indicates a totally different behavior of the function

Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>

wochinge · 2019-10-29T16:04:10Z

@tabergma I took this over from Amogh. I actually think this should not go into 1.4.x but master, what do you think?

tabergma

Looks good 👍 Added a few minor comments.

Yes, let's merge into master.

rasa/model.py

tabergma · 2019-10-30T09:30:04Z

rasa/model.py

+            retrain.core = core_merge_failed
+
+        if not retrain.nlg:
+            retrain.nlg = core_merge_failed


Can you add a comment here why we overwrite retrain.nlg with core_merge_failed?

also changed the code to make it a bit easier to read

rasa/model.py

Co-Authored-By: Tanja <tabergma@gmail.com>

…anular-fingerprint

tanja did a new one

tabergma

🚀 good to go!

rasa/model.py

Co-Authored-By: Tanja <tabergma@gmail.com>

…anular-fingerprint

msamogh force-pushed the granular-fingerprint branch from 7dbdfb8 to bf6c841 Compare September 4, 2019 09:13

msamogh marked this pull request as ready for review September 4, 2019 11:37

msamogh requested a review from tmbo September 4, 2019 11:38

msamogh force-pushed the granular-fingerprint branch from 5e2eec7 to c1473ba Compare September 4, 2019 23:20

msamogh changed the base branch from master to 1.3.x September 5, 2019 11:25

msamogh added 2 commits September 5, 2019 13:26

Avoid retraining Core model if only Templates section is changed

100a1c9

Clean up documentation

482388b

msamogh force-pushed the granular-fingerprint branch from bcd5ff7 to 482388b Compare September 5, 2019 11:26

msamogh and others added 3 commits September 5, 2019 13:33

Update CHANGELOG

eaf077d

Fix underline

1efa655

Merge branch '1.3.x' into granular-fingerprint

7249e78

tmbo previously requested changes Sep 30, 2019

View reviewed changes

msamogh added 4 commits October 4, 2019 13:40

1. Rename replace_templates to retrain_nlg

f43ee78

2. Short-circuit flag before reaching the actual train method

Rename section

1aa84e3

Rename test variables to nlg

7471e85

Black formatting

986a3ac

msamogh requested a review from tmbo October 4, 2019 15:02

tmbo requested review from wochinge and removed request for tmbo October 9, 2019 07:04

wochinge suggested changes Oct 9, 2019

View reviewed changes

msamogh and others added 3 commits October 9, 2019 17:43

Apply suggestions from code review

dcc05ff

Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>

Update rasa/model.py

5cdf99c

Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>

Move into separate function

406c454

wochinge added 6 commits October 29, 2019 16:37

rename

860339b

remove dumping of states

7a1cc9c

set should_retrain_nlg to True in case Core is retrained

e335363

fix failing tests

7a77985

make naming more consistent

a06350e

polishing

a94137d

wochinge requested a review from tabergma October 29, 2019 16:03

remove leading whitespace

2e9fcd7

tabergma approved these changes Oct 30, 2019

View reviewed changes

wochinge and others added 5 commits October 30, 2019 11:12

Apply suggestions from code review

48725df

Co-Authored-By: Tanja <tabergma@gmail.com>

use class based syntax for namedtuple

a811b3d

use functions instead of directly accessing attributes

822d7c9

rename 'merge_model' to 'move_model'

f0dd095

add clarfication comment

b8f3cab

wochinge changed the base branch from 1.4.x to master October 30, 2019 10:31

wochinge added 2 commits October 30, 2019 11:32

Merge branch 'master' into granular-fingerprint

76f27d7

Merge branch 'granular-fingerprint' of github.com:RasaHQ/rasa into gr…

5d01110

…anular-fingerprint

wochinge requested a review from tabergma October 30, 2019 10:33

tabergma approved these changes Oct 30, 2019

View reviewed changes

rasa/model.py Outdated Show resolved Hide resolved

rasa/model.py Outdated Show resolved Hide resolved

wochinge and others added 7 commits October 30, 2019 13:43

black formatting

49229a4

fix variable naming

1d2c79a

Update rasa/model.py

7da43db

Co-Authored-By: Tanja <tabergma@gmail.com>

retrain 'retrain' to 'fingerprint_comparison'

db98356

Merge branch 'granular-fingerprint' of github.com:RasaHQ/rasa into gr…

ce73398

…anular-fingerprint

fix typo in variable name

8956846

Merge branch 'master' into granular-fingerprint

422e08f

wochinge merged commit ff08484 into master Nov 5, 2019

wochinge deleted the granular-fingerprint branch November 5, 2019 08:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid retraining Core model if only templates section is changed #4251

Avoid retraining Core model if only templates section is changed #4251

msamogh commented Aug 15, 2019 •

edited by wochinge

Loading

tmbo commented Sep 3, 2019

msamogh commented Sep 4, 2019

msamogh commented Sep 4, 2019

msamogh commented Sep 4, 2019

msamogh commented Sep 4, 2019

tmbo commented Sep 4, 2019

tmbo left a comment

tmbo Sep 30, 2019

msamogh Oct 4, 2019 •

edited

Loading

wochinge Oct 9, 2019

wochinge Oct 9, 2019

msamogh Oct 22, 2019

msamogh Oct 22, 2019

wochinge left a comment

wochinge Oct 9, 2019

wochinge Oct 9, 2019

wochinge commented Oct 29, 2019

tabergma left a comment

tabergma Oct 30, 2019

wochinge Oct 30, 2019

tabergma left a comment

Avoid retraining Core model if only templates section is changed #4251

Avoid retraining Core model if only templates section is changed #4251

Conversation

msamogh commented Aug 15, 2019 • edited by wochinge Loading

tmbo commented Sep 3, 2019

msamogh commented Sep 4, 2019

msamogh commented Sep 4, 2019

msamogh commented Sep 4, 2019

msamogh commented Sep 4, 2019

tmbo commented Sep 4, 2019

tmbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamogh Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wochinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wochinge commented Oct 29, 2019

tabergma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tabergma left a comment

Choose a reason for hiding this comment

msamogh commented Aug 15, 2019 •

edited by wochinge

Loading

msamogh Oct 4, 2019 •

edited

Loading