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

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

Merged
merged 27 commits into from
Dec 18, 2019

Conversation

erohmensing
Copy link
Contributor

@erohmensing erohmensing commented Nov 25, 2019

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
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? 🤔

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.

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

rasa/cli/interactive.py Outdated Show resolved Hide resolved
@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
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.

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

rasa/cli/interactive.py Outdated Show resolved Hide resolved
rasa/cli/interactive.py Show resolved Hide resolved
rasa/core/training/interactive.py Show resolved Hide resolved
rasa/core/training/interactive.py Outdated Show resolved Hide resolved
rasa/cli/interactive.py Outdated Show resolved Hide resolved
rasa/cli/interactive.py Outdated Show resolved Hide resolved
@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

rasa/cli/interactive.py 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.

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

rasa/cli/interactive.py Outdated Show resolved Hide resolved
rasa/cli/interactive.py Outdated Show resolved Hide resolved
rasa/cli/interactive.py Outdated Show resolved Hide resolved
rasa/cli/interactive.py Outdated Show resolved Hide resolved
rasa/cli/interactive.py Outdated Show resolved Hide resolved
rasa/model.py Outdated Show resolved Hide resolved
tests/cli/test_rasa_interactive.py Outdated Show resolved Hide resolved
tests/cli/test_rasa_interactive.py Outdated Show resolved Hide resolved
@erohmensing erohmensing changed the base branch from 1.5.x to master December 16, 2019 17:56
rasa/importers/rasa.py Outdated Show resolved Hide resolved
@erohmensing
Copy link
Contributor Author

Everything should be addressed 🙂 changed base to master

rasa/cli/interactive.py Outdated Show resolved Hide resolved
rasa/cli/interactive.py 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.

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

rasa/cli/interactive.py Outdated Show resolved Hide resolved
rasa/cli/interactive.py 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.

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

rasa/model.py Show resolved Hide resolved
rasa/core/train.py Outdated Show resolved Hide resolved
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rasa interactive does not work without nlu data
3 participants