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 dependencies in 3.0 to align with rasa-sdk #10667

Merged
merged 10 commits into from
Jan 17, 2022
Merged

Conversation

carlad
Copy link
Contributor

@carlad carlad commented Jan 11, 2022

Proposed changes:
Partially closes #592

This brings dependencies in the pyproject.toml in line with those in rasa-sdk.
I used rasa as the primary dependency source, so most changes were made in the PR for rasa-sdk:
RasaHQ/rasa-sdk#648.

However, as there was a python 3.9 compatibility issue with black, a newer version is being used, hence the changes in this PR.

  • add changelog.
  • Updated formatting.

@carlad carlad requested a review from a team as a code owner January 12, 2022 10:56
@carlad carlad requested review from usc-m and removed request for a team January 12, 2022 10:56
@carlad carlad requested review from a team and koernerfelicia and removed request for a team January 12, 2022 12:13
@carlad carlad requested review from joejuzl and ancalita and removed request for koernerfelicia January 12, 2022 14:05
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

To double-check, python 3.9 requires black higher than v19, so you went ahead and updated this to the version compatible with 3.9?
I would also double-check if there are any style changes that need to be updated in Engineering Guidelines in Notion.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@carlad
Copy link
Contributor Author

carlad commented Jan 12, 2022

@ancalita

To double-check, python 3.9 requires black higher than v19, so you went ahead and updated this to the version compatible with 3.9?

That's correct. When running black I got the following error:

poetry run black --check rasa_sdk tests
Usage: black [OPTIONS] [SRC]...
Try 'black -h' for help.

Error: Invalid value for '-t' / '--target-version': 'py39' is not one of 'py27', 'py33', 'py34', 'py35', 'py36', 'py37', 'py38'.

Some googling revealed it was a bug that was fixed in 21.12 21.x.x

@carlad
Copy link
Contributor Author

carlad commented Jan 12, 2022

@ancalita

I would also double-check if there are any style changes that need to be updated in Engineering Guidelines in Notion.

The only changes made by black were making sure lines didn't exceed 80 columns.

Copy link
Contributor

@usc-m usc-m left a comment

Choose a reason for hiding this comment

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

LGTM

@ancalita
Copy link
Member

The only changes made by black were making sure lines didn't exceed 80 columns.

@carlad Oh is there a way to override the 80 columns to the previous setting, I think it was 88 or 89? This way you won't have such a large diff?

@carlad
Copy link
Contributor Author

carlad commented Jan 12, 2022

@ancalita apologies but I was mistaken. I assumed it was line length, as I didn't realise we were working with an 88 char limit. Black and flake8 are already configured to do an 88 char line length,
see pyproject.toml and setup.cfg

I've had a look through the files again and it seems much of the reformatting has to do with listing arguments on multiple lines, rather than on one line, eg the diff for tests/test_server.py:

 monkeypatch.setattr(
-            sys.modules["rasa.model_training"], "train", mocked_training_function,
+            sys.modules["rasa.model_training"],
+            "train",
+            mocked_training_function,
         )

I looked through the black changelog but couldn't see anything related to this that was changed between v19 and v21.

Would bumping the version cause black to look at all files, rather than just files that had been changed?

@carlad carlad closed this Jan 12, 2022
@carlad carlad reopened this Jan 12, 2022
@joejuzl
Copy link
Contributor

joejuzl commented Jan 13, 2022

Would bumping the version cause black to look at all files, rather than just files that had been changed?

I think black already looks at all the files.
Would be good to keep the same line length I think. We shouldn't have to reformat our whole repo every time we update black (it obscures the git diff a lot too).

@joejuzl
Copy link
Contributor

joejuzl commented Jan 13, 2022

The line length is defined under [tool.black] in pyproject.toml. Maybe it has to be configured differently now?

@carlad
Copy link
Contributor Author

carlad commented Jan 13, 2022

@joejuzl

Would be good to keep the same line length I think. We shouldn't have to reformat our whole repo every time we update black (it obscures the git diff a lot too).

It looks like line length wasn't the issue. Line length is still configured at 88 chars in the pyproject.toml. The reformatting split multiple arguments to multiple lines. Is this a convention?

@joejuzl
Copy link
Contributor

joejuzl commented Jan 13, 2022

It looks like line length wasn't the issue. Line length is still configured at 88 chars in the pyproject.toml. The reformatting split multiple arguments to multiple lines. Is this a convention?

@carlad seems like there's something funky going on here. black should try to keep things on 1 line if they fit - as per the documentation

@carlad
Copy link
Contributor Author

carlad commented Jan 13, 2022

there's something funky going on here.

@joejuzl indeed. Reading through the documentation I can't think of any reason black would be behaving this way except for a lower than default configured line length. Will probe further...

@carlad
Copy link
Contributor Author

carlad commented Jan 14, 2022

Update: This seems like a change in black 21.7b0.
When I use 19.10b0 there's no reformatting. All versions of 21.x apply the reformatting (and v20.x). We need to use v21 as previous versions have a bug when python3.9 is the target version (rasa-sdk has python 3.9 as a target version)
I couldn't find anything obvious in the changelog about this difference in behaviour. So should we just roll with it?

@carlad
Copy link
Contributor Author

carlad commented Jan 14, 2022

Update: Black Black now assumes that if there is a trailing comma for a list of items, all items should be on separate lines. And the new --skip-magic-trailing-comma flag only removes trailing commas. So we might indeed have to roll with it...

@carlad
Copy link
Contributor Author

carlad commented Jan 14, 2022

So in the end, on a new branch, I ran Black with the -skip-magic-trailing-comma flag.
This removed all trailing commas from single lines.
Then I ran black without the flag, and it left the single lines alone.
Then I merged the branch into this one.
Hoping this gives us what we want 🤞 🙏

Copy link
Contributor

@joejuzl joejuzl left a comment

Choose a reason for hiding this comment

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

Looks good 👍
Can we make sure the commits are squashed and the final message makes clear it's a black update and formatting?

@carlad carlad merged commit 36eb9c9 into 3.0.x Jan 17, 2022
@carlad carlad deleted the 592-depen-update-3.0 branch January 17, 2022 14:07
@carlad carlad restored the 592-depen-update-3.0 branch January 17, 2022 15:21
@carlad carlad deleted the 592-depen-update-3.0 branch January 17, 2022 15:29
carlad added a commit that referenced this pull request Jan 18, 2022
* correct transformer_size value if needed

* improve checking and docstring

* var name changed

* domain_for_core_training_provider (#10437)

* domain_for_core_training_provider

* Update rasa/graph_components/providers/domain_for_core_training_provider.py

Co-authored-by: Joe Juzl <[email protected]>

* add integration test; add back enum (in constants.py); replace some text with const; ...

* fix (is_predefined -> is_predefined_type)

* trigger test

* add slot mappings import

* fix validation

* fix

* update domain provider name in schemata; replace str with enum

* update classes in schemata as well...

* trigger tests (MITIE install failed)

* again :)

* fix (slot mapping spec needed for core training)

* lint

* lint..

Co-authored-by: Joe Juzl <[email protected]>

* Update dependencies in 3.0 to align with rasa-sdk (#10667)

* align dependencies
* use black 21.7b0
* apply black and docstring reformatting
* add changelog

* apply black formatting

* fix flake8 check

* update lock file

Co-authored-by: jupyterjazz <[email protected]>
Co-authored-by: Saba Sturua <[email protected]>
Co-authored-by: Kathrin Bujna <[email protected]>
Co-authored-by: Joe Juzl <[email protected]>
Co-authored-by: carlad <[email protected]>
carlad added a commit that referenced this pull request Jan 21, 2022
* correct transformer_size value if needed
* improve checking and docstring
* var name changed
* domain_for_core_training_provider (#10437)
* Update rasa/graph_components/providers/domain_for_core_training_provider.py
Co-authored-by: Joe Juzl <[email protected]>

* add integration test; add back enum (in constants.py); replace some text with const; ...
* fix (is_predefined -> is_predefined_type)
* add slot mappings import
* fix validation
* update domain provider name in schemata; replace str with enum
* update classes in schemata as well...
* fix (slot mapping spec needed for core training)
Co-authored-by: Joe Juzl <[email protected]>

* Update dependencies in 3.0 to align with rasa-sdk (#10667)
* align dependencies
* use black 21.7b0
* apply black and docstring reformatting
* add changelog
* prepare release of version 3.0.5 (#10706)
* bump rasa-sdk

Co-authored-by: jupyterjazz <[email protected]>
Co-authored-by: Saba Sturua <[email protected]>
Co-authored-by: Kathrin Bujna <[email protected]>
Co-authored-by: Joe Juzl <[email protected]>
Co-authored-by: carlad <[email protected]>
carlad added a commit that referenced this pull request Jan 28, 2022
* align dependencies
* use black 21.7b0
* apply black and docstring reformatting
* add changelog
carlad added a commit that referenced this pull request Feb 1, 2022
* Update dependencies in 3.0 to align with rasa-sdk (#10667
* bump rasa-sdk
* filtering messages during training/prediction draft
* remove unfeaturized messages for some nlu components
* update forms docs with dynamic form example for removal of required slot
* Use tf.function for model prediction in RasaModel. (#10738)
* Use prompt_toolkit ^2.0 (#10761)

Co-authored-by: jupyterjazz <[email protected]>
Co-authored-by: carlad <[email protected]>
Co-authored-by: Anca Lita <[email protected]>
Co-authored-by: Matthias Leimeister <[email protected]>
indam23 pushed a commit that referenced this pull request Jul 27, 2022
* align dependencies
* use black 21.7b0
* apply black and docstring reformatting
* add changelog
indam23 pushed a commit that referenced this pull request Jul 27, 2022
* correct transformer_size value if needed

* improve checking and docstring

* var name changed

* domain_for_core_training_provider (#10437)

* domain_for_core_training_provider

* Update rasa/graph_components/providers/domain_for_core_training_provider.py

Co-authored-by: Joe Juzl <[email protected]>

* add integration test; add back enum (in constants.py); replace some text with const; ...

* fix (is_predefined -> is_predefined_type)

* trigger test

* add slot mappings import

* fix validation

* fix

* update domain provider name in schemata; replace str with enum

* update classes in schemata as well...

* trigger tests (MITIE install failed)

* again :)

* fix (slot mapping spec needed for core training)

* lint

* lint..

Co-authored-by: Joe Juzl <[email protected]>

* Update dependencies in 3.0 to align with rasa-sdk (#10667)

* align dependencies
* use black 21.7b0
* apply black and docstring reformatting
* add changelog

* apply black formatting

* fix flake8 check

* update lock file

Co-authored-by: jupyterjazz <[email protected]>
Co-authored-by: Saba Sturua <[email protected]>
Co-authored-by: Kathrin Bujna <[email protected]>
Co-authored-by: Joe Juzl <[email protected]>
Co-authored-by: carlad <[email protected]>
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