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

Enable providing several components of the same type in nlu pipeline #1693

Merged
merged 61 commits into from
Feb 28, 2019

Conversation

Ghostvv
Copy link
Contributor

@Ghostvv Ghostvv commented Feb 5, 2019

Proposed changes:

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

@Ghostvv Ghostvv requested a review from tmbo February 5, 2019 15:35
Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

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

left a couple comments: in general we should make the persistence of policies and components very similar (e.g. right now, every policy gets its own directory wheras for nlu we use file name prefix)

@Ghostvv
Copy link
Contributor Author

Ghostvv commented Feb 6, 2019

core persistence doesn't have metadata persisted

@tmbo
Copy link
Member

tmbo commented Feb 6, 2019

yes I know, every policy does that on its own (which I think might also make sense, because the policies are independent so you can actually train them outside of core and persist them, wheras that doesn't make sense for the components in the nlu pipeline)

@Ghostvv
Copy link
Contributor Author

Ghostvv commented Feb 6, 2019

True, so are we going for folders? Because folders also kind of imply independence, while here only crf and intent classifier are independent

tmbo
tmbo previously approved these changes Feb 22, 2019
Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

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

great job and a lot of work 🚀 couple type and style comments and one additional thing I noticed:

  • mitie_utils and spacy_utils still have the name property

tmbo and others added 18 commits February 22, 2019 18:06
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
@codeclimate
Copy link

codeclimate bot commented Feb 26, 2019

Code Climate has analyzed commit 671f2f3 and detected 16 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 16

View more on Code Climate.

@tmbo tmbo self-requested a review February 27, 2019 10:42
Copy link
Member

@tmbo tmbo 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, some small comments and then we are ready to merge 🚀

@tmbo
Copy link
Member

tmbo commented Feb 27, 2019

@Ghostvv please also make sure to increase the version in the version py and change the min compatible version in the constants.py

Ghostvv and others added 3 commits February 27, 2019 17:07
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
Co-Authored-By: Ghostvv <mr.voov@gmail.com>
@Ghostvv
Copy link
Contributor Author

Ghostvv commented Feb 27, 2019

@tmbo what do you mean with the version? Do we want to release current master 0.15 before this one? and this one should be 0.16?

tmbo and others added 2 commits February 27, 2019 17:10
@tmbo
Copy link
Member

tmbo commented Feb 27, 2019

No I mean increasing the version to 0.15.0a2 and setting the minimum compatible version to that as well, because models with earlier versions can not be loaded anymore, right?

@Ghostvv
Copy link
Contributor Author

Ghostvv commented Feb 27, 2019

@tmbo added changes according to your comments, could you please take a look? Particularly whether migration guide is understandable

@tmbo
Copy link
Member

tmbo commented Feb 27, 2019

looks good

@Ghostvv Ghostvv merged commit fff4826 into master Feb 28, 2019
@Ghostvv Ghostvv deleted the fix-component-config-2 branch February 28, 2019 09:47
taytzehao pushed a commit to taytzehao/rasa that referenced this pull request Jul 14, 2023
Co-authored-by: Jason Song <i@wolfogre.com>
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.

2 participants