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

Deprecate Markdown Training & Test Data #7495

Merged
merged 13 commits into from
Dec 11, 2020
Merged

Deprecate Markdown Training & Test Data #7495

merged 13 commits into from
Dec 11, 2020

Conversation

wochinge
Copy link
Contributor

@wochinge wochinge commented Dec 9, 2020

Proposed changes:

  • deprecate Markdown Training Data (reading and writing)
  • Story reading / writing
  • NLU reading writing
  • Response Selector reading / writing
  • skip printing deprecation warnings when converting training data

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)

@@ -32,11 +32,11 @@ def test_converter_filters_correct_files(training_data_file: Text, should_filter
)


async def test_stories_are_converted(tmpdir: Path):
converted_data_folder = tmpdir / "converted_data"
async def test_stories_are_converted(tmp_path: Path):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced tmpdir as tmpdir is not a Path but of type py.path.local

@wochinge wochinge added this to the 2.2 Rasa Open Source milestone Dec 9, 2020
@wochinge
Copy link
Contributor Author

wochinge commented Dec 9, 2020

@edmacovaz @degiz Re Rasa X:

This change will lead to warnings in the terminal when using Rasa X with Markdown format. I added switches so that it's possible to suppress them, but the question is whether this actually needed. What do you think?
I think the more important TODO item would be to have a warning for the future deprecation in the Rasa X UI.

@wochinge wochinge requested a review from degiz December 9, 2020 10:57
@wochinge wochinge marked this pull request as ready for review December 9, 2020 10:57
@wochinge wochinge requested a review from a team December 9, 2020 10:57
@wochinge wochinge changed the title Deprecate Markdown Training Data Deprecate Markdown Training & Test Data Dec 9, 2020
@wochinge wochinge requested review from edmacovaz and indam23 December 9, 2020 10:58
Copy link
Contributor

@degiz degiz 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, thanks a lot for working on that! 🚀 🚀 🚀
Not putting Approve because of the docs, I'm not sure if the deprecation is communicated well enough.

I added switches so that it's possible to suppress them, but the question is whether this actually needed

I think yes. The way it's used in this PR makes sense to me, because we def. don't want these deprecation warnings when rasa data convert is being used.

future deprecation in the Rasa X UI

This will be more tricky for sure. The closest thing we have now are the warning for the inconsistencies in the domain, and that's a separate endpoint. Probably will have to do something similar with the training data.

@wochinge
Copy link
Contributor Author

wochinge commented Dec 9, 2020

because we def. don't want these deprecation warnings when rasa data convert is being used.

Sorry, I meant if we should use the switches when reading data within Rasa X (e.g. for Integrated Version Control). The deprecation warnings would "only" show up in the server logs which means that only developer can see them. If we use the switches in Rasa X we also have to consider that users might use Rasa X with older Rasa Open Source version where the readers / writer don't have the switch.

@erohmensing
Copy link
Contributor

I think the docs look okay, @degiz what would you add to clarify further?

@degiz
Copy link
Contributor

degiz commented Dec 11, 2020

@erohmensing I don't have anything specific on mind, just wanted anyone from CSE to confirm 👍

@degiz degiz self-requested a review December 11, 2020 12:36
@wochinge wochinge merged commit 28f3ad7 into master Dec 11, 2020
@wochinge wochinge deleted the deprecate-md branch December 11, 2020 13:51
@wochinge
Copy link
Contributor Author

@edmacovaz what do you think about the Rasa X implications?

@wochinge wochinge self-assigned this Dec 11, 2020
@wochinge
Copy link
Contributor Author

wochinge commented Jan 5, 2021

@edmacovaz Have you seen my comment above?

@edmacovaz
Copy link

@wochinge Sorry, I missed that one. I'll try to find some time to think about this once the squad organisation stuff has settled a little. I guess this still falls with you and the Enable team (where I think there's an open question about where you get your design help from). Does that work for you?

@wochinge
Copy link
Contributor Author

wochinge commented Jan 7, 2021

Mhm, could also be the CDD squad 🤔 Works for me 👍🏻

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.

4 participants