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

changed the generator to load full tracker including all sessions + added markers changelog #10170

Merged
merged 9 commits into from
Nov 11, 2021

Conversation

aeshky
Copy link
Contributor

@aeshky aeshky commented Nov 10, 2021

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)

@aeshky aeshky requested a review from ka-bu November 10, 2021 18:52
@aeshky aeshky requested a review from a team as a code owner November 10, 2021 18:52
@aeshky aeshky requested a review from usc-m November 10, 2021 19:01
@aeshky aeshky changed the title changed the generator to load full tracker including all sessions changed the generator to load full tracker including all sessions + added markers changelog Nov 10, 2021
…markers_bugfix

* 'markers_bugfix' of https://github.com/RasaHQ/rasa:
  Fix model dir (#10164)
  add changelog entry about tf2.6 update (#10153)
  conversation_granularity for slack - per user | per user per channel | per user per thread (#10086)
  add changelog fragment for docs changes in 3.0
  fix syntax in training data example in the docs
Copy link
Contributor

@ka-bu ka-bu left a comment

Choose a reason for hiding this comment

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

🚀

tests/core/evaluation/test_marker_tracker_loader.py Outdated Show resolved Hide resolved
tests/core/evaluation/test_marker_tracker_loader.py Outdated Show resolved Hide resolved
tests/core/evaluation/test_marker_tracker_loader.py Outdated Show resolved Hide resolved
tests/core/evaluation/test_marker_tracker_loader.py Outdated Show resolved Hide resolved
tests/core/evaluation/test_marker_tracker_loader.py Outdated Show resolved Hide resolved
tests/core/evaluation/test_marker_tracker_loader.py Outdated Show resolved Hide resolved
rasa/core/evaluation/marker_tracker_loader.py Show resolved Hide resolved
- using tmp_path
- using len() instead of hardcoded number of events.
Copy link
Contributor

@usc-m usc-m left a comment

Choose a reason for hiding this comment

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

LGTM

rasa/core/evaluation/marker_tracker_loader.py Show resolved Hide resolved
@aeshky aeshky enabled auto-merge (squash) November 11, 2021 10:09
@aeshky
Copy link
Contributor Author

aeshky commented Nov 11, 2021

Thanks both for a speedy review 🚀

@@ -20,6 +21,29 @@ def marker_trackerstore() -> TrackerStore:
return store


def test_load_sessions(tmp_path):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def test_load_sessions(tmp_path):
def test_load_sessions(tmp_path : Path):

def test_load_sessions(tmp_path):
"""Tests loading a tracker with multiple sessions."""
domain = Domain.empty()
store = SQLTrackerStore(domain, db=os.path.join(tmp_path, "temp.db"))
Copy link
Contributor

Choose a reason for hiding this comment

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

btw, with paths, you can also do things like

Suggested change
store = SQLTrackerStore(domain, db=os.path.join(tmp_path, "temp.db"))
store = SQLTrackerStore(domain, db= tmp_path / "temp.db")

(https://docs.python.org/3/library/pathlib.html#basic-use)

@aeshky aeshky merged commit 7358f43 into main Nov 11, 2021
@aeshky aeshky deleted the markers_bugfix branch November 11, 2021 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants