Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incremental Training inside Rasa Open Source #7498

Merged
merged 80 commits into from
Dec 15, 2020
Merged

Conversation

dakshvar22
Copy link
Contributor

@dakshvar22 dakshvar22 commented Dec 9, 2020

Proposed changes:

  • Closes Incremental training #6971
  • Adds the ability to load NLU components and Core policies in finetune mode which means they are initialized with a previously trained model and can be further trained on a mix of old and new training data.

Individual PRs:

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)

@dakshvar22
Copy link
Contributor Author

dakshvar22 commented Dec 11, 2020

Okay, so this turned out to be a large PR as well but all the smaller PRs were peer reviewed between @joejuzl @wochinge and @dakshvar22.

@alwx Can you please review the following files - rasa/cli/..., rasa/core/agent.py, rasa/core/train.py, rasa/core/policies/ensemble.py, rasa/model.py, rasa/nlu/model.py, rasa/nlu/train.py, rasa/telemetry.py, rasa/train.py, tests/cli/test_rasa_train.py, tests/conftest.py, tests/core/conftest.py, tests/core/test_agent.py, tests/core/test_ensemble.py, tests/test_train.py, tests/test_model.py ? These are the engineering bits of the PR.

@Ghostvv Would be great if you could glance over - rasa/core/policies/..., rasa/nlu/classifiers/..., rasa/nlu/featurizers/.. , rasa/nlu/constants.py, rasa/nlu/selectors.py, rasa/shared/nlu, rasa/utils/tensorflow/models.py, tests/core/policies, tests/core/test_policies.py, tests/nlu/classifiers/test_diet_classifier.py, tests/nlu/featurizers/..., tests/nlu/selectors/, tests/shared/nlu/training_data/test_training_data.py. Most of these were reviewd by Tanja as part of this PR - #7419)

@dakshvar22 dakshvar22 requested review from alwx and Ghostvv December 11, 2020 11:37
Copy link
Contributor

@Ghostvv Ghostvv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good from my side. left a couple of comments

changelog/6971.feature.md Show resolved Hide resolved
docs/docs/components.mdx Outdated Show resolved Hide resolved
docs/docs/components.mdx Outdated Show resolved Hide resolved
rasa/utils/tensorflow/models.py Outdated Show resolved Hide resolved
tests/core/policies/test_rule_policy.py Show resolved Hide resolved
@dakshvar22 dakshvar22 requested a review from Ghostvv December 13, 2020 20:57
def add_force_param(
parser: Union[argparse.ArgumentParser, argparse._ActionsContainer]
) -> None:
"""Specifies if the model should be trained from scratch."""
Copy link
Contributor

@alwx alwx Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Args section is missing in the docstring but that's something CI would probably complain about.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's missing on purpose because I don't see any value in having full docstrings for these helper functions. The CI actually allows one-line docstrings 😁 What do you think?

Copy link
Contributor

@Ghostvv Ghostvv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the files that I was assigned to look good

@@ -419,7 +422,8 @@ def test_train_core_help(run: Callable[..., RunResult]):
[--augmentation AUGMENTATION] [--debug-plots] [--force]
[--fixed-model-name FIXED_MODEL_NAME]
[--percentages [PERCENTAGES [PERCENTAGES ...]]]
[--runs RUNS]"""
[--runs RUNS] [--finetune [FINETUNE]]
[--epoch-fraction EPOCH_FRACTION]"""
Copy link
Contributor

@alwx alwx Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no tests for the command itself, right? I think it makes sense to add them.
(something with run_in_simple_project("train", "--finetune"))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done a basic cli test with the command (which just tests the arg is actually passed down). And I've changed test_model_finetuning to not mock the train methods and actually run the second training.

@dakshvar22
Copy link
Contributor Author

A final run of model regression tests before merge is running here

@dakshvar22 dakshvar22 requested a review from alwx December 14, 2020 18:05
@dakshvar22 dakshvar22 merged commit 9f94cbc into master Dec 15, 2020
@dakshvar22 dakshvar22 deleted the continuous_training branch December 15, 2020 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incremental training
5 participants