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.

Sign up for GitHub

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

Jump to bottom

let rasa interactive automatically do rasa interactive core when no nlu data #4834

Merged
merged 27 commits into from
Dec 18, 2019
Merged

let rasa interactive automatically do rasa interactive core when no nlu data #4834

merged 27 commits into from
Dec 18, 2019

Conversation

erohmensing
Copy link
Contributor

@erohmensing erohmensing commented Nov 25, 2019 •
edited
Loading

Proposed changes:

  • interactive now follows the same logic as train does, by letting train decide what gets trained and loaded based on the data provided. (unless it is training a new model with rasa interactive core in which case it will only train a core model regardless of provided data)
  • interactive training now explicitly does not run on or train nlu-only models. Before, it broke.
  • fix rasa interactive does not work without nlu data #4799

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality
  • updated the documentation
  • updated the changelog
  • reformat files using black (please check Readme for instructions)

@erohmensing erohmensing requested a review from wochinge November 25, 2019 16:59
@erohmensing
Copy link
Contributor Author

I guess there is really no rasa interactive nlu, what do you think it should do if only nlu data is loaded? 🤔

wochinge
wochinge reviewed Nov 25, 2019
View reviewed changes
Copy link
Contributor

@wochinge wochinge left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for tackling this! We also need a changelog update and ideally a test

@erohmensing
Copy link
Contributor Author

Changelog will wait until there's a proper bug branch open 😃Any suggestion on testing? It's hard with CLI stuff, as i don't think we currently test the actual functionality of loading + starting up the interactive server, it's just internal methods

@erohmensing erohmensing changed the base branch from master to 1.5.x November 26, 2019 08:48
@erohmensing erohmensing requested a review from wochinge November 28, 2019 14:51
wochinge
wochinge suggested changes Nov 29, 2019
View reviewed changes
Copy link
Contributor

@wochinge wochinge left a comment •
edited by erohmensing
Loading

Choose a reason for hiding this comment

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

Nice, a lot of cleaning up as well 💯

  • If no stories are present, then we should not train a NLU only model, cause this will also not help. Right?

  • Can we remove lines 1621 to 1624 in training/interactive? I think that case can't happen.

  • And tests would be great, at least for

    • no core data given
    • optional: if training function is called when no model is given

@tmbo
Copy link
Member

tmbo commented Dec 6, 2019

@erohmensing what are the next steps on this one?

@erohmensing
Copy link
Contributor Author

Sorry, just getting time to come around to it was the next step 😅

@tmbo
Copy link
Member

tmbo commented Dec 10, 2019

no worries, just wanted to make sure it is still on your radar so we can make sure it doesn't get stale

@erohmensing erohmensing requested a review from wochinge December 11, 2019 23:21
erohmensing
erohmensing commented Dec 13, 2019
View reviewed changes
wochinge
wochinge suggested changes Dec 16, 2019
View reviewed changes
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 with the testing! 🥇

  • Can you please change it to use the RasaFileImporter
  • please make sure to merge it in the right branch in case you don't get this done until Wednesday

@erohmensing erohmensing changed the base branch from 1.5.x to master December 16, 2019 17:56
erohmensing
erohmensing commented Dec 16, 2019
View reviewed changes
@erohmensing
Copy link
Contributor Author

Everything should be addressed 🙂 changed base to master

wochinge
wochinge reviewed Dec 17, 2019
View reviewed changes
wochinge
wochinge reviewed Dec 17, 2019
View reviewed changes
@erohmensing erohmensing requested a review from wochinge December 17, 2019 12:20
wochinge
wochinge reviewed Dec 17, 2019
View reviewed changes
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.

not done, I think github is in a weird state 😀

wochinge
wochinge approved these changes Dec 17, 2019
View reviewed changes
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.

Let's discuss the comments, but otherwise good to go! Thanks for making this so much more robust!

@erohmensing erohmensing mentioned this pull request Dec 17, 2019
Closed
@tmbo tmbo merged commit 701958b into master Dec 18, 2019
@tmbo tmbo deleted the interactive-core branch December 18, 2019 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmbo tmbo tmbo left review comments

@wochinge wochinge wochinge approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rasa interactive does not work without nlu data
3 participants
@erohmensing @tmbo @wochinge