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

Switch to TF 2.0 and new NLU components #5266

Merged
merged 829 commits into from
Feb 26, 2020
Merged

Switch to TF 2.0 and new NLU components #5266

merged 829 commits into from
Feb 26, 2020

Conversation

tabergma
Copy link
Contributor

@tabergma tabergma commented Feb 19, 2020

Proposed changes:

  • Switched to Tensorflow 2.0
  • Added new NLU components, such as DIETClassifier
  • Deprecated some old NLU components, such as CRFEntityExtractor and KerasPolicy

(We still need to finish up some parts of the documentation and some minor changes inside the code itself. Test time has increased quite a bit and we will try to reduce it, but this is not our highest priority for now. This can be also tackled after the merge if needed.)

Status (please check what you already did):

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

@tabergma tabergma requested review from akelad and tmbo February 19, 2020 16:23
@tmbo
Copy link
Member

tmbo commented Feb 19, 2020

Test time has increased quite a bit and we will try to reduce it, but this is not our highest priority for now.

I don't think doubling the test time is something we can accept - I do not think we should merge this before looking into this.

@wochinge
Copy link
Contributor

Just had a glimpse, but we should add docstrings to at least all classes and public functions / methods

@tabergma tabergma mentioned this pull request Feb 26, 2020
3 tasks
@tabergma
Copy link
Contributor Author

Created some issues to tackle remaining cleanup tasks after the merge:

@Ghostvv
Copy link
Contributor

Ghostvv commented Feb 26, 2020

@wochinge regarding attributes: we kept the way it was done before, just substituted magic strings with constants. I agree it is not very clear from the constants how they are used, but as soon as you see then in context, in my opinion it is quite clear, and additional wording just make it more complicated.

I agree that the whole message attribute logic is not very intuitive, but changing it is out of the scope of this paper PR

@degiz degiz requested a review from akelad February 26, 2020 10:47
Ghostvv and others added 4 commits February 26, 2020 13:51
- It's possible now to specify region name for AWS S3
- Tests are setting mock region name now
@suzhaomin666
Copy link

how to use bert chinese

@wochinge
Copy link
Contributor

@suzhaomin666 Please ask questions in the Rasa Forum. This link should hopefully also help you: https://rasa.com/docs/rasa/nlu/components/#hftransformersnlp

@yuxuan2015
Copy link

@suzhaomin666 This link should hopefully also help you: https://github.com/lhr0909/rasa-v2-nlu-bert-chinese

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.