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

Make Rasa X model pull interval configurable in local mode #4635

Merged
merged 46 commits into from
Nov 26, 2019
Merged

Conversation

btotharye
Copy link
Contributor

Proposed changes:

  • Updated the x.py CLI file to give a warning if we are about to modify the endpoints.yml and also actually use the wait_time_between_pulls from the source endpoints.yml file.

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality - will do this after proposed changes are signed off
  • updated the changelog
  • reformat files using black (please check Readme for instructions)

@btotharye btotharye requested a review from wochinge October 18, 2019 08:43
@btotharye
Copy link
Contributor Author

let me know what you think @wochinge this is just the first round, although I don't see Rasa X replacing my config if I put custom stuff in my endpoints.yml file

Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Logic looks good! I left a couple of suggestions to polish it a bit. Can we please also write test(s)?

rasa/cli/x.py Outdated Show resolved Hide resolved
rasa/cli/x.py Outdated Show resolved Hide resolved
rasa/cli/x.py Outdated Show resolved Hide resolved
rasa/cli/x.py Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

👍

CHANGELOG.rst Outdated Show resolved Hide resolved
rasa/cli/x.py Outdated Show resolved Hide resolved
rasa/cli/x.py Outdated Show resolved Hide resolved
@btotharye
Copy link
Contributor Author

ok @wochinge I think all your recent feedback has been implemented now.

@btotharye
Copy link
Contributor Author

fixing the linting issue, forgot to run that before pushing up

@btotharye
Copy link
Contributor Author

ok @wochinge all your requested changes should be updated and I fixed the changelog conflict, all tests are passing let me know any other feedback

Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

looks great! Can you please also write unit test(s) for it?
Also, this pr should probably go against 1.4.x (please make sure your changelog entries end up in the section)

rasa/cli/x.py Outdated Show resolved Hide resolved
@btotharye
Copy link
Contributor Author

looks great! Can you please also write unit test(s) for it?
Also, this pr should probably go against 1.4.x (please make sure your changelog entries end up in the section)

Yep I was gonna write the tests after we agreed on how it all is which we do now so I'll work on that next and fix the changelog

@btotharye
Copy link
Contributor Author

will be working on the tests today/tomorrow to finish this up

@erohmensing
Copy link
Contributor

@btotharye can you give the PR a more descriptive name while you're at it?

@btotharye btotharye changed the title Fix for #4524 Make Rasa X model pull interval configurable in local mode Oct 28, 2019
@btotharye
Copy link
Contributor Author

@btotharye can you give the PR a more descriptive name while you're at it?

done

CHANGELOG.rst Outdated Show resolved Hide resolved
rasa/cli/x.py Outdated Show resolved Hide resolved
@btotharye btotharye requested a review from wochinge November 22, 2019 12:00
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Great work!

@btotharye
Copy link
Contributor Author

Great work!

Any idea @wochinge why this is failing now? All I did was update it from the recent master updates

@btotharye
Copy link
Contributor Author

pretty sure I got the issues resolved if this all passes @wochinge I'll merge it

@btotharye
Copy link
Contributor Author

btotharye commented Nov 25, 2019

Looks like it was a simple thing to fix @wochinge I'm pushing up the changes these tests should pass now.

CHANGELOG.rst Outdated Show resolved Hide resolved
@btotharye
Copy link
Contributor Author

re-running tests from the update @wochinge if they pass which they should merging this one

@btotharye
Copy link
Contributor Author

@wochinge can we merge this one? I'm not sure why the tests are showing funky, on the site it shows coverage increased.

@wochinge
Copy link
Contributor

@btotharye Now you deleted your own changelog 😆

@btotharye
Copy link
Contributor Author

@btotharye Now you deleted your own changelog 😆

ha let me double check it, crap

@btotharye
Copy link
Contributor Author

ok @wochinge the changelog should be updated now sorry about that

@wochinge
Copy link
Contributor

@btotharye Please put it in the changelog for 1.6.0a1 since 1.5.0 was already released

@btotharye
Copy link
Contributor Author

sorry fixed wasn't paying attention there I was in 1.5.x mode

CHANGELOG.rst Outdated Show resolved Hide resolved
@wochinge wochinge merged commit 4eba702 into master Nov 26, 2019
@wochinge wochinge deleted the issue_4524 branch November 26, 2019 17:00
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.

4 participants