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

File importer #4014

Merged
merged 43 commits into from Jul 23, 2019
Merged

File importer #4014

merged 43 commits into from Jul 23, 2019

Conversation

wochinge
Copy link
Contributor

@wochinge wochinge commented Jul 15, 2019

Proposed changes:

  • fixes Customizable file import component #3937
  • move SkillSelector to rasa/data/skill (I messed up the git rename, I'm sorry!)
  • updated SkillSelector to use TrainingFileImporter interface
  • new interface TrainingFileImporter which centralizes the file importing
  • made nlu training asynchronously
  • updated training data validator to use TrainingFileImporter

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)

@wochinge
Copy link
Contributor Author

@tabergma Would be great to get some first feedback from you!
There are a couple of more things we could base on the new interface, but maybe it makes sense to tackle this in another issue (interactive learning, some testing parts).

@wochinge
Copy link
Contributor Author

I will fix the remaining tests tomorrow

@@ -0,0 +1,188 @@
import logging
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I messed up the renaming here, sorry! I removed all the static stuff and adapted it to the TrainingFileImporter interface.

@tabergma
Copy link
Contributor

This PR is huge. Can we somehow split it up? Maybe just introduce the new importers in the first PR and move the actual usage of them into another PR?

Copy link
Contributor

@tabergma tabergma left a comment

Choose a reason for hiding this comment

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

Just looked at the files in importer so far. Also want to test it locally. Will continue later. Left a couple of comments so far.

rasa/importers/importer.py Outdated Show resolved Hide resolved
async def get_story_data(
self,
interpreter: "NaturalLanguageInterpreter" = RegexInterpreter(),
template_variables: Optional[Dict] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

What are template_variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akelad Do you have idea? I did some digging in the code and apparently you can specify some templates in stories which are replaced while reading the files?

rasa/importers/importer.py Outdated Show resolved Hide resolved
rasa/importers/importer.py Show resolved Hide resolved
rasa/importers/importer.py Outdated Show resolved Hide resolved
rasa/importers/importer.py Outdated Show resolved Hide resolved
rasa/importers/utils.py Show resolved Hide resolved
rasa/importers/skill.py Show resolved Hide resolved
rasa/cli/interactive.py Show resolved Hide resolved
rasa/core/validator.py Outdated Show resolved Hide resolved
@wochinge
Copy link
Contributor Author

This PR is huge. Can we somehow split it up? Maybe just introduce the new importers in the first PR and move the actual usage of them into another PR?

@tabergma Sorry, I know it's painful 😞 But just adding the importers would not make them usable at all. Only putting them in the train process makes them usable at all 🤔

rasa/importers/importer.py Outdated Show resolved Hide resolved
rasa/importers/importer.py Show resolved Hide resolved
rasa/importers/importer.py Outdated Show resolved Hide resolved
rasa/importers/skill.py Show resolved Hide resolved
rasa/importers/skill.py Outdated Show resolved Hide resolved
rasa/model.py Show resolved Hide resolved
rasa/nlu/config.py Outdated Show resolved Hide resolved
rasa/train.py Outdated Show resolved Hide resolved
@tabergma
Copy link
Contributor

@wochinge I added a couple of more comments. Still need to test it locally. Will do it tomorrow. But in general, I like the idea and the implementation looks good.
There is only one thing I'm not sure about (also added a comment): Where are the constructor arguments of the importers set? Currently, it looks like they need to be specified in the config file itself.

@wochinge wochinge marked this pull request as ready for review July 19, 2019 11:19
@wochinge
Copy link
Contributor Author

@tabergma Ready for a final review
@erohmensing @MetcalfeTom Do you have some input on the docs part of the pr?

@erohmensing
Copy link
Contributor

Content looks good, thanks for the full example! Will do a quick copy edit and fix the docs warnings while i'm at it (check those next time 😉)

rasa/importers/importer.py Outdated Show resolved Hide resolved
@wochinge
Copy link
Contributor Author

If anybody has input for better names for TrainingFileImporter and SimpleFileImporter I'd be very happy. Not really satisfied with the naming atm.

@MetcalfeTom
Copy link
Contributor

Docs be fine 👍

docs/api/training-data-importers.rst Show resolved Hide resolved
rasa/core/agent.py Outdated Show resolved Hide resolved
rasa/core/domain.py Show resolved Hide resolved
rasa/importers/importer.py Outdated Show resolved Hide resolved
rasa/importers/importer.py Outdated Show resolved Hide resolved
rasa/importers/skill.py Outdated Show resolved Hide resolved
rasa/importers/skill.py Outdated Show resolved Hide resolved
tests/test_importer.py Outdated Show resolved Hide resolved
Copy link
Contributor

@tabergma tabergma left a comment

Choose a reason for hiding this comment

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

Works as expected 🚀 Great work!

@wochinge wochinge merged commit 7e1fe61 into master Jul 23, 2019
@wochinge wochinge deleted the file-importer branch July 23, 2019 09:16
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.

Customizable file import component
4 participants