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

Implement train_step instead of overwriting fit #7438

Merged
merged 42 commits into from
Mar 10, 2021
Merged

Implement train_step instead of overwriting fit #7438

merged 42 commits into from
Mar 10, 2021

Conversation

tabergma
Copy link
Contributor

@tabergma tabergma commented Dec 2, 2020

Proposed changes:

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)

rasa/utils/tensorflow/models.py Outdated Show resolved Hide resolved
rasa/utils/train_utils.py Show resolved Hide resolved
rasa/core/policies/ted_policy.py Show resolved Hide resolved
rasa/utils/tensorflow/callback.py Outdated Show resolved Hide resolved
rasa/utils/tensorflow/data_generator.py Outdated Show resolved Hide resolved
rasa/utils/tensorflow/models.py Outdated Show resolved Hide resolved
rasa/utils/tensorflow/models.py Outdated Show resolved Hide resolved
@tabergma
Copy link
Contributor Author

tabergma commented Dec 11, 2020

The results of the model regression tests do not match the results I got when I'm testing locally. Need to investigate that further.

@joejuzl
Copy link
Contributor

joejuzl commented Jan 20, 2021

I profiled the memory usage while running tests/core/test_policies.py on master, and on this branch.

master:

policy_memory_profile_master2

train_step (commit 4ea8d8 - current head )

policy_memory_profile_train_step2

train_step (commit 07d63c - before keras clear_session)

policy_memory_profile_train_step_before_keras_clear

Observations:

  • Takes much longer to run on train_step.
  • Way higher memory usage on train_step, even at the same relative point in time.
  • The Keras clear_session seemed to help somewhat.
  • All show a continuous increase in memory usage, suggesting a memory leak.

Base automatically changed from master to main January 22, 2021 11:15
@Ghostvv
Copy link
Contributor

Ghostvv commented Feb 2, 2021

@joejuzl could you please give it a final review?

@Ghostvv Ghostvv requested a review from joejuzl February 2, 2021 12:13
.github/workflows/continous-integration.yml Outdated Show resolved Hide resolved
rasa/core/policies/ted_policy.py Outdated Show resolved Hide resolved
@joejuzl
Copy link
Contributor

joejuzl commented Mar 3, 2021

#8085 should help to reduce the memory usage of the policy tests and stop the CI tests failing.

@Ghostvv
Copy link
Contributor

Ghostvv commented Mar 9, 2021

@joejuzl thank you for updating the tests. Could you please merge your changes into this branch? There are some merge conflicts

@Ghostvv Ghostvv enabled auto-merge (squash) March 10, 2021 13:06
@Ghostvv Ghostvv merged commit 46c1a97 into main Mar 10, 2021
@Ghostvv Ghostvv deleted the train_step branch March 10, 2021 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools:clear-poetry-cache-unit-tests Clear poetry cache for the unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants