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

Fix Interactive story counter #8086

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

rafaelteodosio
Copy link
Contributor

@rafaelteodosio rafaelteodosio commented Mar 2, 2021

Proposed changes:

  • Fix bug Interactive story counter doesn't update
  • Resolve #7462

Status:

  • 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)

joaoCeilandia and others added 6 commits February 20, 2021 03:40
Co-authored-by: Rafael Teodosio <rafzteodosio@gmail.com>
Co-authored-by: joao vitor silva <joaovitordmrosa@hotmail.com>
Co-authored-by: Rafael Teodosio <rafzteodosio@gmail.com>
Co-authored-by: Rafael Teodosio <rafzteodosio@gmail.com>
Co-authored-by: Rafael Teodosio <rafzteodosio@gmail.com>
@CLAassistant
Copy link

CLAassistant commented Mar 2, 2021

CLA assistant check
All committers have signed the CLA.

tmbo
tmbo previously requested changes Mar 2, 2021
Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the fix on the interactive learning dumping. Do you mind taking a look at adding a test and a changelog entry? Let me know if you need pointers 🙂

with open(
export_story_path, "r", encoding=rasa.shared.utils.io.DEFAULT_ENCODING
) as f:
i = sum(1 for s in f if 'interactive_story_' in s) + 1 # count the times string 'interactive story' occur in file
Copy link
Member

Choose a reason for hiding this comment

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

let's make interactive_story_ a constant to ensure it stays the same between this line and L831

with open(
export_story_path, "r", encoding=rasa.shared.utils.io.DEFAULT_ENCODING
) as f:
i = sum(1 for s in f if 'interactive_story_' in s) + 1 # count the times string 'interactive story' occur in file
Copy link
Member

Choose a reason for hiding this comment

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

while we are here, let's give that i a more expressive name

@@ -816,10 +816,15 @@ def _write_stories_to_file(
else:
append_write = "w" # make a new file if not

with open(
Copy link
Member

Choose a reason for hiding this comment

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

do you mind adding a test to check this will work in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rafaelteodosio and i don't know how make the requested test. @tmbo could you give us some advices?

Copy link
Contributor

Choose a reason for hiding this comment

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

Look here tests/core/training/test_interactive.py. You can write a test that exports twice and assert that the correct count is used for both.

@rafaelteodosio
Copy link
Contributor Author

Ok @tmbo , @joaoCeilandia and I will make the requested changes, thanks for your time.

@sara-tagger sara-tagger requested a review from indam23 March 3, 2021 07:00
@tmbo tmbo removed the request for review from indam23 March 3, 2021 09:32
joaoCeilandia and others added 3 commits March 7, 2021 04:19
…able

Co-authored-by: Rafael Teodosio <rafzteodosio@gmail.com>
Co-authored-by: joao vitor silva <joaovitordmrosa@hotmail.com>
Co-authored-by: Rafael Teodosio <rafzteodosio@gmail.com>
@rafaelteodosio
Copy link
Contributor Author

rafaelteodosio commented Mar 7, 2021

@tmbo We made the requested changes.

@joaoCeilandia
Copy link
Contributor

I don't know if we could do this, but can i request your review here @melindaloubser1? We need this for a subject of our college.

@indam23
Copy link
Contributor

indam23 commented Mar 12, 2021

I think since @tmbo has been following this PR, it's best if he does a final review of it. It needs a test to be written as well.

@joaoCeilandia
Copy link
Contributor

Ok, thanks for your time and support @melindaloubser1 .

@RasaHQ RasaHQ deleted a comment from sara-tagger Apr 13, 2021
@tmbo tmbo requested review from joejuzl and removed request for tmbo April 14, 2021 09:07
Copy link
Contributor

@joejuzl joejuzl left a comment

Choose a reason for hiding this comment

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

Hey @rafaelteodosio - any progress with this?

@stale
Copy link

stale bot commented Apr 16, 2022

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 16, 2022
@tmbo tmbo requested a review from a team as a code owner September 12, 2022 19:48
@tmbo tmbo removed the request for review from a team September 12, 2022 19:48
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.

Interactive story counter doesn't update
6 participants