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

#7329 load models in finetune mode core #7458

Merged
merged 18 commits into from
Dec 9, 2020

Conversation

joejuzl
Copy link
Contributor

@joejuzl joejuzl commented Dec 4, 2020

Proposed changes:
Load the core models in fine-tuning mode.

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)

@joejuzl joejuzl marked this pull request as draft December 4, 2020 16:14
Copy link
Contributor

@dakshvar22 dakshvar22 left a comment

Choose a reason for hiding this comment

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

Haven't reviewed extensively yet, but an initial observation.

rasa/core/policies/ensemble.py Outdated Show resolved Hide resolved
rasa/core/policies/ensemble.py Outdated Show resolved Hide resolved
rasa/core/policies/ensemble.py Outdated Show resolved Hide resolved
rasa/core/policies/ensemble.py Outdated Show resolved Hide resolved
rasa/core/policies/policy.py Show resolved Hide resolved
rasa/core/policies/policy.py Outdated Show resolved Hide resolved
rasa/core/policies/policy.py Outdated Show resolved Hide resolved
rasa/core/policies/rule_policy.py Outdated Show resolved Hide resolved
rasa/core/policies/sklearn_policy.py Outdated Show resolved Hide resolved
rasa/train.py Show resolved Hide resolved
tests/core/test_ensemble.py Show resolved Hide resolved
@joejuzl joejuzl marked this pull request as ready for review December 7, 2020 15:29
rasa/core/policies/ensemble.py Outdated Show resolved Hide resolved
rasa/core/policies/ensemble.py Outdated Show resolved Hide resolved
rasa/core/policies/policy.py Outdated Show resolved Hide resolved
rasa/core/policies/ensemble.py Outdated Show resolved Hide resolved
rasa/core/policies/memoization.py Outdated Show resolved Hide resolved
rasa/core/policies/policy.py Outdated Show resolved Hide resolved
rasa/core/policies/policy.py Outdated Show resolved Hide resolved
rasa/core/policies/policy.py Outdated Show resolved Hide resolved
rasa/core/policies/rule_policy.py Outdated Show resolved Hide resolved
rasa/core/policies/sklearn_policy.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dakshvar22 dakshvar22 left a comment

Choose a reason for hiding this comment

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

Looking great 👍

rasa/core/policies/ensemble.py Outdated Show resolved Hide resolved
rasa/core/policies/policy.py Outdated Show resolved Hide resolved
rasa/core/policies/sklearn_policy.py Outdated Show resolved Hide resolved
Comment on lines +503 to +506
meta["should_finetune"] = kwargs.get("should_finetune", False)
if "epoch_override" in kwargs:
meta[EPOCHS] = kwargs["epoch_override"]

Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a larger change which i'll address in my PR as part of making core policies finetunable. It's okay to keep as it is here.

joejuzl and others added 2 commits December 8, 2020 16:41
Co-authored-by: Daksh Varshneya <d.varshneya@rasa.com>
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

🚀

rasa/core/policies/ensemble.py Outdated Show resolved Hide resolved
rasa/core/policies/ensemble.py Outdated Show resolved Hide resolved
rasa/core/policies/ensemble.py Outdated Show resolved Hide resolved
rasa/core/policies/ensemble.py Outdated Show resolved Hide resolved
rasa/core/policies/ensemble.py Outdated Show resolved Hide resolved
tests/core/test_ensemble.py Outdated Show resolved Hide resolved
tests/core/test_policies.py Outdated Show resolved Hide resolved
tests/core/test_policies.py Outdated Show resolved Hide resolved
tests/core/test_policies.py Show resolved Hide resolved
tests/test_train.py Outdated Show resolved Hide resolved
joejuzl and others added 3 commits December 8, 2020 17:09
@dakshvar22
Copy link
Contributor

@joejuzl Can you add the required changes to custom policies in the migration guide as part of this PR itself?

@wochinge
Copy link
Contributor

wochinge commented Dec 8, 2020

I think a constant for should_finetune would be great considering the many locations where we use this. Can in my opinion also be a separate PR then.

@joejuzl
Copy link
Contributor Author

joejuzl commented Dec 9, 2020

@dakshvar22 I think a separate PR for the migration guide would make more sense so we can start using this code easier in other PRs.

@joejuzl joejuzl merged commit 241a075 into continuous_training Dec 9, 2020
@joejuzl joejuzl deleted the 7329/load_models_in_finetune_mode_core branch December 9, 2020 10:40
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.

3 participants