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

Update prompt-toolkit, ruff, opentelemetry-exporter-jaeger #1051

Closed
wants to merge 3 commits into from

Conversation

tabergma
Copy link
Contributor

@tabergma tabergma commented Dec 1, 2023

Proposed changes:
Update

  • prompt-toolkit
  • ruff
  • opentelemetry-exporter-jaeger

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality
  • updated the documentation in the rasaHQ/rasa
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@tabergma tabergma changed the title Update pep440-version-utils Update pep440-version-utils, prompt-toolkit, ruff, opentelemetry-exporter-jaeger Dec 1, 2023
@tabergma tabergma changed the title Update pep440-version-utils, prompt-toolkit, ruff, opentelemetry-exporter-jaeger Update prompt-toolkit, ruff, opentelemetry-exporter-jaeger Dec 5, 2023
@tabergma
Copy link
Contributor Author

tabergma commented Dec 5, 2023

Should this PR target main or 3.7.x?

@@ -79,13 +79,13 @@ coloredlogs = ">=10,<16"
sanic = "^21.12.0"
typing-extensions = ">=4.1.1,<5.0.0"
Sanic-Cors = "^2.0.0"
prompt-toolkit = "^3.0,<3.0.29"
Copy link
Member

Choose a reason for hiding this comment

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

This update has to be validated with rasa-private CLI commands (like rasa interactive or rasa shell), because they had introduced a breaking change in 3.0.30 which I'm not sure they addressed.

"ruamel.yaml" = ">=0.16.5,<0.18.0"
websockets = ">=10.0,<12.0"
pluggy = "^1.0.0"
opentelemetry-api = "~1.15.0"
opentelemetry-sdk = "~1.15.0"
opentelemetry-exporter-jaeger = "~1.15.0"
Copy link
Member

Choose a reason for hiding this comment

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

Two points here, if we're updating this dependency, all the other open telemetry dependencies have to be on the same minor. In addition, we have to align this dependency version with the identical dependencies in rasa-plus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ancalita for pointing it out. As it seems to be much more work than expected, I would close this PR and create an issue for it. We are currently focusing on a project that needs to be done by end of the year.

@ancalita
Copy link
Member

ancalita commented Dec 5, 2023

Should this PR target main or 3.7.x?

Housekeeping PRs should always target main, only dependency vulnerability fixes should target release branches.

@tabergma
Copy link
Contributor Author

tabergma commented Dec 5, 2023

As there is much more work than expected, we will tackle it later (https://rasahq.atlassian.net/browse/ENG-718)

@tabergma tabergma closed this Dec 5, 2023
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