Async Tracker Store Support #10696

danielenricocahall · 2022-01-18T00:47:39Z

This is an effort to make tracker stores asynchronous, based off of the work initially done in this PR and it should address [this issue] (#8774). I've tried to account for the comments made in the original PR and reduce the surface area/object wrapping from the original implementation, but that comes at a trade-off - specifically making the other tracker store methods async.

Also, should we convert some of the tracker store implementations to use async APIs (i.e; AIORedis for AioRedis, Motor for Mongo)?

. First PR, so entirely open to feedback!

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)

CLAassistant · 2022-01-18T00:47:47Z

All committers have signed the CLA.

… async-trackerstore-support

joejuzl · 2022-03-21T12:57:45Z

Hey @wochinge @joejuzl could someone possibly retrigger the CI at earliest convenience? Sorry to be a bother, just really excited to get this in :)

Hey @danielenricocahall sorry I was on holiday :) - have re-run it now.
Also, in-case you weren't aware, you can use make to run the various checks locally, e.g. make lint (check the Makefile for the full list). This may make it easier to get all issues fixed locally.

danielenricocahall · 2022-03-21T13:01:24Z

Hey @wochinge @joejuzl could someone possibly retrigger the CI at earliest convenience? Sorry to be a bother, just really excited to get this in :)

Hey @danielenricocahall sorry I was on holiday :) - have re-run it now. Also, in-case you weren't aware, you can use make to run the various checks locally, e.g. make lint (check the Makefile for the full list). This may make it easier to get all issues fixed locally.

Hey @joejuzl no worries and thank you!!! And ah yes good to know - I was running a few commands manually based on what I saw in CI but the make commands are definitely easier. Let me resolve this other linting problem...

danielenricocahall · 2022-03-21T13:29:38Z

Hey @joejuzl I've corrected most of the linting errors after running make lint-docstring. However, there are a few errors I don't understand:

make lint-docstrings
git diff main...HEAD -- rasa | poetry run flake8 --select D --diff
/usr/local/lib/python3.8/dist-packages/setuptools/command/install.py:34: SetuptoolsDeprecationWarning: setup.py install is deprecated. Use build and pip and other standards-based tools.
  warnings.warn(
rasa/core/tracker_store.py:277:1: D205 1 blank line required between summary line and description
rasa/core/tracker_store.py:277:1: D415 First line should end with a period, question mark, or exclamation point
rasa/core/tracker_store.py:1253:1: D102 Missing docstring in public method
rasa/core/tracker_store.py:1260:1: D102 Missing docstring in public method
rasa/core/tracker_store.py:1267:1: D102 Missing docstring in public method
make: *** [Makefile:95: lint-docstrings] Error 1

When I navigate to these lines, they are methods which have not had any docstring modifications in this PR - only the signatures have changed (to add async). Should I still make changes to those docstrings?

joejuzl · 2022-03-21T13:36:10Z

When I navigate to these lines, they are methods which have not had any docstring modifications in this PR - only the signatures have changed (to add async). Should I still make changes to those docstrings?

The check uses rather broad strokes to determine what needs to be checked, and since it has not been around since the start it often requires small changes to existing docstrings. Let me know if you need a hand with any of them (it should be fine to keep them fairly succinct and simple.)

danielenricocahall · 2022-03-21T13:42:05Z

When I navigate to these lines, they are methods which have not had any docstring modifications in this PR - only the signatures have changed (to add async). Should I still make changes to those docstrings?

The check uses rather broad strokes to determine what needs to be checked, and since it has not been around since the start it often requires small changes to existing docstrings. Let me know if you need a hand with any of them (it should be fine to keep them fairly succinct and simple.)

Done! :)

danielenricocahall · 2022-03-21T14:21:41Z

When I navigate to these lines, they are methods which have not had any docstring modifications in this PR - only the signatures have changed (to add async). Should I still make changes to those docstrings?

The check uses rather broad strokes to determine what needs to be checked, and since it has not been around since the start it often requires small changes to existing docstrings. Let me know if you need a hand with any of them (it should be fine to keep them fairly succinct and simple.)

Done! :)

Oof, then those changes caused Flake8 to complain about line length errors 😵 . Just addressed and pushed up.

danielenricocahall · 2022-03-21T15:49:25Z

Just corrected the typing errors. With the two AwaitableTrackerStore methods (retrieve and keys), I added ignore[return-value] since the alternative would be to change the type signature for those methods for all TrackerStore subclasses, which I thought was less clean (especially if the idea is that AwaitableTrackerStore would be removed in 4.0 since all tracker store implementations should be asynchronous by then and there would be no need for the wrapper).

… async-trackerstore-support

danielenricocahall · 2022-03-22T11:07:41Z

Hey @joejuzl it looks like all but one test are passing 🥳 - test_train_status_is_not_blocked_by_training in test-other-unit-tests when doing multi-process tests. Do you have any suggestions on mitigating that issue?

joejuzl

💯 Fantastic job! Thanks for this 🚀 🚀 🚀

joejuzl · 2022-03-22T12:08:30Z

Hey @joejuzl it looks like all but one test are passing 🥳 - test_train_status_is_not_blocked_by_training in test-other-unit-tests when doing multi-process tests. Do you have any suggestions on mitigating that issue?

Looks like it's a random failure - re-running it now.

danielenricocahall · 2022-03-22T12:09:02Z

Hey @joejuzl it looks like all but one test are passing 🥳 - test_train_status_is_not_blocked_by_training in test-other-unit-tests when doing multi-process tests. Do you have any suggestions on mitigating that issue?

Looks like it's a random failure - re-running it now.

Thank you!

danielenricocahall · 2022-03-22T12:28:35Z

Hey @joejuzl it looks like all but one test are passing 🥳 - test_train_status_is_not_blocked_by_training in test-other-unit-tests when doing multi-process tests. Do you have any suggestions on mitigating that issue?

Looks like it's a random failure - re-running it now.

Thank you!

Looks like everything passed now but the codeclimate report update failed.

joejuzl · 2022-03-22T12:31:43Z

Looks like everything passed now but the codeclimate report update failed.

Don't worry about that job.
Do you have the privileges to merge?
If so make sure to squash, you can then either keep the default squashed message or write something more succinct.
If not, I can merge for you.

danielenricocahall · 2022-03-22T12:33:03Z

Looks like everything passed now but the codeclimate report update failed.

Don't worry about that job. Do you have the privileges to merge? If so make sure to squash, you can then either keep the default squashed message or write something more succinct. If not, I can merge for you.

Okay great! Unfortunately, I do not have privileges to merge.

danielenricocahall · 2022-03-22T12:46:00Z

Hey @joejuzl would the next step be to migrate some of the existing tracker stores to using async lib equivalents e.g; MongoTrackerStore -> motor, etc.? I wouldn't mind doing that in the future.

Make `TrackerStore` interface methods asynchronous and supply an `AwaitableTrackerstore` wrapper for custom tracker stores which do not implement the methods as asynchronous. Squashed commits: * refactor tracker store and tests to be async * update core modules with async tracker store calls * update server with async tracker store calls * await tracker store call in twilio voice * add await in test rasa export * add awaits to test_agent for tracker store * add awaits to tracker store functions in processor tests * refactor exporter tests for async tracker store * use asyncmock from unittest instead of custom * add async in test_rasa_export * fixture update for async tracker store * update marker logic to handle async tracker store * fix mark tracker loader tests for async tracker store * add awaits to server and server tests * add await to dialogue test with tracker store * add await to tracker test * formatting in tracker store * more formatting fixes * more formatting fixes * address formatting changes * change return type and remove awaitable tracker store wrapper in create * make stream_events async * address comments (remove redundant methods in awaitable tracker store + raise exception) * make _run_markers and _run_markers_cli sync to ensure CLI can be run * add warning and test for creating async tracker store from endpoint config * add changelog entry * use TrackerStore instead of "TrackerStore" in typehint Co-authored-by: Joe Juzl <joejuzl@gmail.com> * use TrackerStore instead of "TrackerStore" in typehint Co-authored-by: Joe Juzl <joejuzl@gmail.com> * change user warning to deprecation warning * fix typo in comment * have fallback_tracker_store return in memory tracker store without awaitable wrapper * import async mock from conftest instead of unittest to suport Python 3.7 * delete unused imports in marker_tracker_loader * apply black to modules which failed ci linter * resolve line length linting in tracker_store.py * refer to request.app.ctx.agent object instead of request.app.agent * resolve ci failures from not adding async/await * applied black to reformat three modules failing code quality * correct most docstring linting errors * fix docstring linting errors * fix flake8 line length errors * fix mypy type checking errors * linting corrections after adding type ignores to methods * delete extra periods in docstring Co-authored-by: Joe Juzl <joejuzl@gmail.com>

danielenricocahall added 12 commits January 17, 2022 14:58

refactor tracker store and tests to be async

5dd0050

update core modules with async tracker store calls

c31e2b4

update server with async tracker store calls

650d98c

await tracker store call in twilio voice

797db3d

add await in test rasa export

d4f1882

add awaits to test_agent for tracker store

f096668

add awaits to tracker store functions in processor tests

bc38edb

refactor exporter tests for async tracker store

57e3e71

use asyncmock from unittest instead of custom

0968977

add async in test_rasa_export

eb38a7f

fixture update for async tracker store

dc9852e

update marker logic to handle async tracker store

06acad5

danielenricocahall requested a review from a team as a code owner January 18, 2022 00:47

danielenricocahall requested review from jupyterjazz and removed request for a team January 18, 2022 00:47

danielenricocahall and others added 14 commits January 17, 2022 19:54

Merge branch 'main' into async-trackerstore-support

4606718

fix mark tracker loader tests for async tracker store

9dc30cb

Merge remote-tracking branch 'origin/async-trackerstore-support' into…

6f46794

… async-trackerstore-support

add awaits to server and server tests

10e14bc

add await to dialogue test with tracker store

ade9ab9

add await to tracker test

036038a

Merge branch 'main' into async-trackerstore-support

3c6d353

formatting in tracker store

a81346b

more formatting fixes

b281549

more formatting fixes

c3bf506

address formatting changes

5ad4523

change return type and remove awaitable tracker store wrapper in create

d57cc05

make stream_events async

16992e2

Merge branch 'main' into async-trackerstore-support

749bbca

joejuzl mentioned this pull request Mar 21, 2022

Fix mypy misc #11003

Merged

4 tasks

correct most docstring linting errors

5c6a0b9

fix docstring linting errors

9af5999

fix flake8 line length errors

12892bd

danielenricocahall added 2 commits March 21, 2022 11:29

fix mypy type checking errors

39478dc

linting corrections after adding type ignores to methods

4787db4

danielenricocahall and others added 3 commits March 21, 2022 12:09

Merge branch 'main' into async-trackerstore-support

090c1f2

delete extra periods in docstring

9f49ba7

Merge remote-tracking branch 'origin/async-trackerstore-support' into…

4ccf56a

… async-trackerstore-support

joejuzl approved these changes Mar 22, 2022

View reviewed changes

joejuzl merged commit ca316fc into RasaHQ:main Mar 22, 2022

danielenricocahall deleted the async-trackerstore-support branch March 22, 2022 16:46

danielenricocahall mentioned this pull request Mar 24, 2022

Make Mongo tracker store async #11026

Closed

4 tasks

indam23 mentioned this pull request Aug 1, 2022

Async trackerstore support #8774

Closed

3 tasks

danielenricocahall mentioned this pull request Sep 7, 2022

Make Redis Tracker Store Async #11550

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async Tracker Store Support #10696

Async Tracker Store Support #10696

danielenricocahall commented Jan 18, 2022 •

edited

Loading

CLAassistant commented Jan 18, 2022 •

edited

Loading

joejuzl commented Mar 21, 2022

danielenricocahall commented Mar 21, 2022

danielenricocahall commented Mar 21, 2022

joejuzl commented Mar 21, 2022

danielenricocahall commented Mar 21, 2022

danielenricocahall commented Mar 21, 2022

danielenricocahall commented Mar 21, 2022 •

edited

Loading

danielenricocahall commented Mar 22, 2022 •

edited

Loading

joejuzl left a comment

joejuzl commented Mar 22, 2022

danielenricocahall commented Mar 22, 2022 •

edited

Loading

danielenricocahall commented Mar 22, 2022

joejuzl commented Mar 22, 2022

danielenricocahall commented Mar 22, 2022

danielenricocahall commented Mar 22, 2022

Async Tracker Store Support #10696

Async Tracker Store Support #10696

Conversation

danielenricocahall commented Jan 18, 2022 • edited Loading

CLAassistant commented Jan 18, 2022 • edited Loading

joejuzl commented Mar 21, 2022

danielenricocahall commented Mar 21, 2022

danielenricocahall commented Mar 21, 2022

joejuzl commented Mar 21, 2022

danielenricocahall commented Mar 21, 2022

danielenricocahall commented Mar 21, 2022

danielenricocahall commented Mar 21, 2022 • edited Loading

danielenricocahall commented Mar 22, 2022 • edited Loading

joejuzl left a comment

Choose a reason for hiding this comment

joejuzl commented Mar 22, 2022

danielenricocahall commented Mar 22, 2022 • edited Loading

danielenricocahall commented Mar 22, 2022

joejuzl commented Mar 22, 2022

danielenricocahall commented Mar 22, 2022

danielenricocahall commented Mar 22, 2022

danielenricocahall commented Jan 18, 2022 •

edited

Loading

CLAassistant commented Jan 18, 2022 •

edited

Loading

danielenricocahall commented Mar 21, 2022 •

edited

Loading

danielenricocahall commented Mar 22, 2022 •

edited

Loading

danielenricocahall commented Mar 22, 2022 •

edited

Loading