Flag to use CPU on TEDPolicy train #10944

WashingtonBispo · 2022-02-24T02:43:29Z

Proposed changes:

Add flag to disable training on GPU for TEDPolicy #10897

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)

Bumps [google-github-actions/setup-gcloud](https://github.com/google-github-actions/setup-gcloud) from 0.4.0 to 0.5.1. - [Release notes](https://github.com/google-github-actions/setup-gcloud/releases) - [Changelog](https://github.com/google-github-actions/setup-gcloud/blob/master/CHANGELOG.md) - [Commits](google-github-actions/setup-gcloud@e0f83f2...04141d8) --- updated-dependencies: - dependency-name: google-github-actions/setup-gcloud dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [actions/github-script](https://github.com/actions/github-script) from 4.0.2 to 6. - [Release notes](https://github.com/actions/github-script/releases) - [Commits](actions/github-script@a3e7071...9ac0880) --- updated-dependencies: - dependency-name: actions/github-script dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [pytest-timeout](https://github.com/pytest-dev/pytest-timeout) from 1.4.2 to 2.1.0. - [Release notes](https://github.com/pytest-dev/pytest-timeout/releases) - [Commits](pytest-dev/pytest-timeout@1.4.2...2.1.0) --- updated-dependencies: - dependency-name: pytest-timeout dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>

…e-github-actions-setup-gcloud-0.5.1 Bump google-github-actions/setup-gcloud from 0.4.0 to 0.5.1

….1.0 Bump pytest-timeout from 1.4.2 to 2.1.0

…ns-github-script-6 Bump actions/github-script from 4.0.2 to 6

…tions-actions-github-script-6 Revert "Bump actions/github-script from 4.0.2 to 6"

…tions-google-github-actions-setup-gcloud-0.5.1 Revert "Bump google-github-actions/setup-gcloud from 0.4.0 to 0.5.1"

Co-authored-by: Emily <[email protected]>

CLAassistant · 2022-02-24T02:43:34Z

All committers have signed the CLA.

indam23

I'll leave it to @kedz to provide a more thorough review, just noted two things to address for now

rasa/core/policies/ted_policy.py

indam23 · 2022-03-02T10:35:07Z

@SamuelNoB Could you please sign the contributor license agreement in the meantime?
@WashingtonBispo Do the comments left here so far make sense?

rasa/core/policies/ted_policy.py

Co-authored-by: Emily <[email protected]>

dakshvar22

Thanks for the quick fixes. I have made some more suggestions + a couple of things that are pending:

We need to make sure that inference also runs on the requested device. In order to do that, we can wrap the model loading call with the appropriate context manager as you have done in train. The value of USE_GPU would have been persisted during training and will be loaded correctly in the config variable (line 1067), so you can use config to fetch the value of USE_GPU.
Please add a description of use_gpu to the table of parameters of TEDPolicy and UnexpecTEDIntentPolicy
Please add a changelog describing the change made in this PR to the changelog folder. Please refer to the README inside that folder for instructions on the content of the changelog file.

pyproject.toml

rasa/core/policies/ted_policy.py

rasa/core/policies/unexpected_intent_policy.py

Co-authored-by: Emily <[email protected]>

WashingtonBispo · 2022-03-05T00:14:27Z

I'm not sure if i know how to make this first request.

pyproject.toml

ka-bu

The following still needs to be implemented to make sure that we also switch to CPU when loading and finetuning an existing model:

We need to make sure that inference also runs on the requested device. In order to do that, we can wrap the model loading call with the appropriate context manager as you have done in train. The value of USE_GPU would have been persisted during training and will be loaded correctly in the config variable (line 1067), so you can use config to fetch the value of USE_GPU.

Some more details on that:

If we want to use the CPU for finetuning, then we need to wrap the model = cls._load_tf_model(...) call that Daksh referenced above into a with tf.device(...) as well (because that call will call the RasaModel load function which contains the compile that builds the graph for the loaded model and which will put it on the GPU if not told otherwise)
Since that call happens in a class function, we don't have access to self.config as before and need to get the information whether we want to use the GPU or not from somewhere else. We can get the same information from the config object that is created a few lines above the model = cls._load_tf_model(...) call.

Let me know if this helps or if you have more questions

Co-authored-by: Emily <[email protected]>

dakshvar22 · 2022-03-21T10:54:36Z

@WashingtonBispo Can you please merge main into your branch and remove the changes you have made to poetry.lock?

emysdias · 2022-03-22T00:50:50Z

@WashingtonBispo Can you please merge main into your branch and remove the changes you have made to poetry.lock?

Now it is fixed

rasa/core/policies/ted_policy.py

changelog/10897.improvement.md

dakshvar22 · 2022-03-22T18:17:07Z

Hi @WashingtonBispo @emysdias Added a couple of final suggestions + the code quality check is failing above. Can you please address these?

Co-authored-by: WashingtonBispo <[email protected]>

emysdias · 2022-03-22T19:34:18Z

Hi @WashingtonBispo @emysdias Added a couple of final suggestions + the code quality check is failing above. Can you please address these?

done

rasa/core/policies/unexpected_intent_policy.py

dakshvar22 · 2022-03-22T19:57:58Z

Changes needed still

emysdias · 2022-03-22T20:13:12Z

Changes needed still

ops, I'm going to change

Co-authored-by: WashingtonBispo <[email protected]>

dakshvar22 · 2022-03-23T12:56:37Z

Running final model regression tests on this PR

dakshvar22 · 2022-03-24T06:57:25Z

hi @emysdias and @WashingtonBispo , all tests seem to be passing now. Can you please merge the base branch once more and then I can enable auto-merge 🎉

emysdias · 2022-03-24T10:52:33Z

hi @emysdias and @WashingtonBispo , all tests seem to be passing now. Can you please merge the base branch once more and then I can enable auto-merge tada

done

dakshvar22

Looks great! Thanks a lot for your contribution ✨

emysdias · 2022-03-24T11:45:10Z

Looks great! Thanks a lot for your contribution sparkles

Thank you a lot

WashingtonBispo · 2022-03-24T11:55:23Z

Looks great! Thanks a lot for your contribution ✨

Thank you sir.

dependabot bot and others added 11 commits February 16, 2022 23:43

Merge pull request #197 from FGA-GCES/dependabot-github_actions-googl…

384ba6d

…e-github-actions-setup-gcloud-0.5.1 Bump google-github-actions/setup-gcloud from 0.4.0 to 0.5.1

Merge pull request #203 from FGA-GCES/dependabot-pip-pytest-timeout-2…

761a0b1

….1.0 Bump pytest-timeout from 1.4.2 to 2.1.0

Merge pull request #198 from FGA-GCES/dependabot-github_actions-actio…

0e4c64c

…ns-github-script-6 Bump actions/github-script from 4.0.2 to 6

Revert "Bump google-github-actions/setup-gcloud from 0.4.0 to 0.5.1"

c502f1a

Revert "Bump actions/github-script from 4.0.2 to 6"

13c59ab

Merge pull request #213 from FGA-GCES/revert-198-dependabot-github_ac…

10d18c4

…tions-actions-github-script-6 Revert "Bump actions/github-script from 4.0.2 to 6"

Merge pull request #212 from FGA-GCES/revert-197-dependabot-github_ac…

ae61b2c

…tions-google-github-actions-setup-gcloud-0.5.1 Revert "Bump google-github-actions/setup-gcloud from 0.4.0 to 0.5.1"

Aded flag that allow use CPU instead of GPU on TEDPolicy train

71d22e4

Co-authored-by: Emily <[email protected]>

WashingtonBispo requested review from a team and kedz and removed request for a team February 24, 2022 02:43

Merge branch 'main' of git://github.com/RasaHQ/rasa into use-gpu

8749b36

indam23 reviewed Feb 24, 2022

View reviewed changes

rasa/core/policies/ted_policy.py Outdated Show resolved Hide resolved

rasa/core/policies/ted_policy.py Outdated Show resolved Hide resolved

indam23 requested review from dakshvar22 and removed request for kedz March 2, 2022 10:31

dakshvar22 reviewed Mar 2, 2022

View reviewed changes

rasa/core/policies/ted_policy.py Outdated Show resolved Hide resolved

ka-bu reviewed Mar 2, 2022

View reviewed changes

rasa/core/policies/ted_policy.py Outdated Show resolved Hide resolved

WashingtonBispo and others added 2 commits March 2, 2022 11:17

Make changes request.

9518a30

Co-authored-by: Emily <[email protected]>

Remove useless commands

971d77d

Co-authored-by: Emily <[email protected]>

dakshvar22 requested changes Mar 4, 2022

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

rasa/core/policies/ted_policy.py Outdated Show resolved Hide resolved

rasa/core/policies/ted_policy.py Outdated Show resolved Hide resolved

rasa/core/policies/unexpected_intent_policy.py Outdated Show resolved Hide resolved

Making requested changes

ed99d28

Co-authored-by: Emily <[email protected]>

ka-bu reviewed Mar 7, 2022

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

ka-bu reviewed Mar 7, 2022

View reviewed changes

WashingtonBispo mentioned this pull request Mar 7, 2022

Flag to use CPU on TEDPolicy train FGA-GCES/rasa#226

Open

Revert changes on dependencies files

609e9ac

Co-authored-by: Emily <[email protected]>

Merge branch 'main' of https://github.com/RasaHQ/rasa into use-gpu

cbad9bd

dakshvar22 reviewed Mar 22, 2022

View reviewed changes

rasa/core/policies/ted_policy.py Outdated Show resolved Hide resolved

dakshvar22 reviewed Mar 22, 2022

View reviewed changes

changelog/10897.improvement.md Outdated Show resolved Hide resolved

fix code quality

09870ba

Co-authored-by: WashingtonBispo <[email protected]>

dakshvar22 reviewed Mar 22, 2022

View reviewed changes

rasa/core/policies/unexpected_intent_policy.py Outdated Show resolved Hide resolved

remove line

a47558a

Co-authored-by: WashingtonBispo <[email protected]>

dakshvar22 added runner:gpu status:model-regression-tests and removed runner:gpu status:model-regression-tests labels Mar 23, 2022

Merge branch 'main' of https://github.com/RasaHQ/rasa into use-gpu

7e464ef

Merge branch 'main' of https://github.com/RasaHQ/rasa into use-gpu

7b88148

dakshvar22 enabled auto-merge (squash) March 24, 2022 11:37

dakshvar22 self-requested a review March 24, 2022 11:38

dakshvar22 approved these changes Mar 24, 2022

View reviewed changes

dakshvar22 merged commit 43b6b9a into RasaHQ:main Mar 24, 2022

emysdias deleted the use-gpu branch March 24, 2022 12:23

dakshvar22 mentioned this pull request Mar 24, 2022

Add flag to disable training on GPU for TEDPolicy #10897

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flag to use CPU on TEDPolicy train #10944

Flag to use CPU on TEDPolicy train #10944

WashingtonBispo commented Feb 24, 2022

CLAassistant commented Feb 24, 2022 •

edited

Loading

indam23 left a comment

indam23 commented Mar 2, 2022

dakshvar22 left a comment

WashingtonBispo commented Mar 5, 2022

ka-bu left a comment

dakshvar22 commented Mar 21, 2022

emysdias commented Mar 22, 2022

dakshvar22 commented Mar 22, 2022

emysdias commented Mar 22, 2022

dakshvar22 commented Mar 22, 2022

emysdias commented Mar 22, 2022

dakshvar22 commented Mar 23, 2022

dakshvar22 commented Mar 24, 2022

emysdias commented Mar 24, 2022

dakshvar22 left a comment

emysdias commented Mar 24, 2022

WashingtonBispo commented Mar 24, 2022

Flag to use CPU on TEDPolicy train #10944

Flag to use CPU on TEDPolicy train #10944

Conversation

WashingtonBispo commented Feb 24, 2022

CLAassistant commented Feb 24, 2022 • edited Loading

indam23 left a comment

Choose a reason for hiding this comment

indam23 commented Mar 2, 2022

dakshvar22 left a comment

Choose a reason for hiding this comment

WashingtonBispo commented Mar 5, 2022

ka-bu left a comment

Choose a reason for hiding this comment

dakshvar22 commented Mar 21, 2022

emysdias commented Mar 22, 2022

dakshvar22 commented Mar 22, 2022

emysdias commented Mar 22, 2022

dakshvar22 commented Mar 22, 2022

emysdias commented Mar 22, 2022

dakshvar22 commented Mar 23, 2022

dakshvar22 commented Mar 24, 2022

emysdias commented Mar 24, 2022

dakshvar22 left a comment

Choose a reason for hiding this comment

emysdias commented Mar 24, 2022

WashingtonBispo commented Mar 24, 2022

CLAassistant commented Feb 24, 2022 •

edited

Loading