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

Remove experimental label for entity roles and groups #8791

Closed
4 tasks done
TyDunn opened this issue Jun 1, 2021 · 29 comments
Closed
4 tasks done

Remove experimental label for entity roles and groups #8791

TyDunn opened this issue Jun 1, 2021 · 29 comments
Assignees
Labels
area:rasa-oss 🎡 Anything related to the open source Rasa framework area:rasa-oss/ml/nlu-components Issues focused around rasa's NLU components type:docs 📖 Improvements to the documenation. Adding missing pieces or improving existing ones. type:maintenance 🔧 Improvements to tooling, testing, deployments, infrastructure, code style.

Comments

@TyDunn
Copy link
Contributor

TyDunn commented Jun 1, 2021

We have added support to entity roles and groups in Rasa X. We should now remove the experimental feature designation in the Rasa Open Source docs. More context in Slack.

https://rasa.com/docs/rasa/nlu-training-data#entities-roles-and-groups

Definition of done

  • Remove experimental warning in docs
  • Remove the warning in the code
  • Update the experimental feature table in Notion
  • Draft a forum announcement with @EmmaWightman
@TyDunn TyDunn added type:docs 📖 Improvements to the documenation. Adding missing pieces or improving existing ones. type:maintenance 🔧 Improvements to tooling, testing, deployments, infrastructure, code style. area:rasa-oss 🎡 Anything related to the open source Rasa framework area:rasa-oss/ml/nlu-components Issues focused around rasa's NLU components labels Jun 1, 2021
@TyDunn TyDunn changed the title Remove experimental label entity roles and groups Remove experimental label for entity roles and groups Jun 1, 2021
@samsucik
Copy link
Contributor

This also includes announcing the news in the forum, right?

@TyDunn
Copy link
Contributor Author

TyDunn commented Jun 11, 2021

That's a really good question. Wdyt @karen-white @EmmaWightman?

@karen-white
Copy link
Contributor

Sounds good to me! I think we'd want to make this a newsletter mention as well - either in June or July depending on the timing. cc @Cooper-lower

@EmmaWightman
Copy link
Contributor

sounds good @TyDunn will prepare something for it

@alopez
Copy link
Contributor

alopez commented Jul 23, 2021

assignee: @aeshky
reviewer: @koernerfelicia

@aeshky
Copy link
Contributor

aeshky commented Jul 24, 2021

Working branch: remove_exp_label.
Notion page updated: here.
Pull request:
#9194 this PR got too messy when I changed the target branch.
I created a new PR here: #9237

@aeshky
Copy link
Contributor

aeshky commented Jul 26, 2021

@EmmaWightman I have updated the code, docs, and notion page. Are you available to draft a forum announcement with me? What's the best way to arrange this?

We are also moving the story validation tool from an experimental to an official feature (#8024 ). Do we need to draft an announcement for that too?

@TyDunn
Copy link
Contributor Author

TyDunn commented Jul 26, 2021

We are also moving the story validation tool from an experimental to an official feature (#8024 ). Do we need to draft an announcement for that too?

@aeshky I don't think we do since I believe almost all users / customers already use it. We should probably post in Slack and let the CSE / Dev Rel teams know though

@aeshky
Copy link
Contributor

aeshky commented Jul 26, 2021

@TyDunn when is the next Rasa release? @EmmaWightman and I want to make sure the annoucement is ready before then.

@TyDunn
Copy link
Contributor Author

TyDunn commented Jul 26, 2021

@aeshky I think we can cut a patch release with these changes as soon as it is merged and you are ready. There are no minors until 3.0, but this does not include enhancements, so I think we can just do it in 2.8.x

@wochinge
Copy link
Contributor

@TyDunn I think removing an experimental label (although there are no changes other than removing the warning) is nothing we should do in a micro. I'd vote for merging to main as it's neither a bugfix nor a documentation improvement. This makes also more sense to me for the announcement since "Feature xy is now no longer experimental in the last bugfix release" sounds wrong 😆

@TyDunn
Copy link
Contributor Author

TyDunn commented Jul 28, 2021

I think of it more as "Feature xy is now no longer experimental as of Rasa Open Source 2.8.4". It gives us something to talk about / share with the community and customers during this couple month period of no minors and does not require us to wait three months for people to start using these features we consider generally available now. Curious what some others think @m-vdb @scottruitt

@wochinge
Copy link
Contributor

I understand the motivation but in my opinion it breaks the purpose of semantic versioning and the common understanding of it. Especially enterprises might even see it as an indicator for fragility / lack of maturity when we suddenly start shipping features in micro releases.

Currently (with some exceptions 🙄 ) they can trust us that a new micro release will only contain fixes and does not contain potentially breaking changes (they won't know that we internally just remove the warning - in my opinion they will see the changelog item and might be wary of potential breaking changes)

@TyDunn
Copy link
Contributor Author

TyDunn commented Jul 28, 2021

But the experimental label is concept we have made up ourselves. We are not shipping any new features / code. Just removing a warning that is holding back many users from trying a feature that already exists

@wochinge
Copy link
Contributor

We are not shipping any new features / code. Just removing a warning [...]

I know but our customers / users don't. If we do this, then I'd make it very clear in the changelog that we just remove a warning and any behavior remains unchanged.

@m-vdb
Copy link
Collaborator

m-vdb commented Jul 28, 2021

I agree with what Tobias says here, I think there is a risk with doing this. Is there an in-between solution? We could frame it as:

  • with 3.0, the feature is no longer experimental
  • explain what this means for the codebase (= no changes)
  • explain what this means to users

That way, people who want to already start using it can do so before the 3.0 release, if the announcement gives them the guarantees they need?

@TyDunn
Copy link
Contributor Author

TyDunn commented Jul 28, 2021

@m-vdb Are you saying only merge the warning removal into main but still announce it now, telling them to ignore the warning in the 2.8 version they will be using?

@m-vdb
Copy link
Collaborator

m-vdb commented Jul 28, 2021

yes, somewhat: I think users ignoring the warning is a consequence of us explaining what the removal entails. We can be explicit and clear in the announcement and tell people they can ignore the warning. We could even go one step further and modify the warning itself to tell them it's gonna be resolved in 3.0 (and ship that change in a micro if needed, that wouldn't hurt I think). Something like this in the docs: "Heads up! This feature will no longer be experimental in 3.0. Read more about t his here (link to announcement)"?

@aeshky
Copy link
Contributor

aeshky commented Jul 28, 2021

Thank you all for your input. I'm seeing two options:

Option 1:
Merge to 2.8.x, but make it very clear in the change logs that the warning message is the only change.
Announce to the community.

Option 2:
Merge to main.
Create a second pull request to change the warning message to say that the feature will no longer be experimental in 3.0, and merge to 2.8.x.
Announce that the features will no longer be experimental in 3.0.
When releasing 3.0, announce again that the features are no longer experimental.

I am leaning towards 1. If we say the feature will no longer be experimental in 3.0 then people might not use it until then, when in practice nothing besides the wording of the message will change.

@scottruitt
Copy link

I found this issue on SemVer's repo to be less conclusive than I would have liked (almost seven years later and still no resolution): semver/semver#238 tl;dr: it doesn't seem like we're breaking any rules if we remove it without a major version bump, but it also doesn't seem like the rules are clear enough to know for sure.

@m-vdb
Copy link
Collaborator

m-vdb commented Jul 29, 2021

if we're going down this path, should we at least document what is an experimental feature to our users in the docs, why we need it and what that means for them, especially when we remove an experimental flag?

@TyDunn
Copy link
Contributor Author

TyDunn commented Jul 29, 2021

@m-vdb Adam and I are working on improving our experimental feature approach here

@EmmaWightman
Copy link
Contributor

text for community:


Previously, we introduced Entity Roles & Groups in Rasa Open Source 1.10 as an experimental feature.

Due to the positive feedback we have received from our community, from release version [please add release version] it will be considered a fully integrated feature in Rasa Open Source. Therefore, we have also added full support for this feature in Rasa X. [please add release version]

If you want to learn more about it, please check our documentation here.

@aeshky
Copy link
Contributor

aeshky commented Jul 29, 2021

If I change the docs in main when do the changes go live? Just wondering if what I mentioned in option 2 above is correct.

Also, who should make the final decision on this? 🙂

@m-vdb
Copy link
Collaborator

m-vdb commented Jul 29, 2021

you'll need to change 2.8.x and do a micro release for the default documentation to update

@TyDunn
Copy link
Contributor Author

TyDunn commented Jul 29, 2021

I think we should iterate quickly here and go with option 1, making it very clear in the changelog that we just remove a warning / any behavior remains unchanged and knowing that we are revisiting our approach to experimental features (i.e. going forward we plan to define the conditions necessary for an experimental feature to become generally available and share this publicly in the docs). @m-vdb @wochinge how does that sound?

@m-vdb
Copy link
Collaborator

m-vdb commented Jul 29, 2021

Since we have next steps to address the concerns that Tobias and I voiced, I think it makes sense to go with option 1. Let's be pragmatic and merge this 🚀

@aeshky
Copy link
Contributor

aeshky commented Jul 30, 2021

Thank you all!
I'm keen to merge today if possible, but had to create a new PR since the old branch was based on main (things got messey when I switched the target branch to 2.8.x). Tobi already approved the old PR (here) but he's offline today. @m-vdb any chance you can approve it instead? 🙂

@aeshky aeshky closed this as completed Jul 30, 2021
@aeshky aeshky reopened this Jul 30, 2021
@aeshky
Copy link
Contributor

aeshky commented Jul 30, 2021

@EmmaWightman
The release version for Rasa Open Source is 2.8.2 (Micro release PR here)
The Rasa X version is 0.40.0 (according to this comment)

@aeshky aeshky closed this as completed Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:rasa-oss 🎡 Anything related to the open source Rasa framework area:rasa-oss/ml/nlu-components Issues focused around rasa's NLU components type:docs 📖 Improvements to the documenation. Adding missing pieces or improving existing ones. type:maintenance 🔧 Improvements to tooling, testing, deployments, infrastructure, code style.
Projects
None yet
Development

No branches or pull requests

10 participants