-
Notifications
You must be signed in to change notification settings - Fork 906
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
[KED-3043] Add support for Python 3.10 #1275
Conversation
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
1433e71
to
868f52c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
On the parallelisation of unit tests: I don't really mind the slowdown, but it would be possible to run all of them in parallel apart from 3.10. You'd need to add a when/unless
clause into the unit_tests
job a bit like we have here (only in main
, not develop
): https://github.com/kedro-org/kedro/blob/main/.circleci/continue_config.yml#L231-L244
Co-authored-by: Antony Milne <49395058+AntonyMilneQB@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome LGTM! 🎉
Makefile
Outdated
pytest tests --cov-config pyproject.toml | ||
|
||
test-no-spark: | ||
pytest tests --no-cov --ignore tests/extras/datasets/spark --numprocesses 4 --dist loadfile | ||
pytest tests --no-cov --ignore tests/extras/datasets/spark |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were quite handy in making our tests run slightly faster by parallelising. What's the reason for removing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See point 1: #1275 (comment), I could implement Antony's suggestion: #1275 (review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I didn't read carefully enough! I think we should implement some conditions like @AntonyMilneQB suggested or have different moto
requirements (whichever is easier). 4 mins is a long time and we were struggling with slow builds before, I'd love it if we could avoid regressing on this. 🙏
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Laurens Vijnck <laurens_vijnck@mckinsey.com>
Description
Add support for Python 3.10
Development notes
Significant things to note:
moto
had to be updated to a newer version, however when the tests are run in parallel eithertest_incremental_dataset
ortest_partioned_dataset
would fail intermittently, so I changed the tests to not be run in parallel anymore. This might not be an ideal solution, because it adds run time of about 4 mins to each unit test build. The lower version ofmoto
could still be used for Python versions < 3.10, but the 3.10 build would then still fail if run in parallel.matplotlib>=3.5
is needed, which breaksholoviews
. I've disabled the holoviews tests for Python 3.10, but also had to ignore that dataset from the test coverage check inpyproject.toml
.pylint
repo: Issues accessingPurePath.parents
by index in Python 3.10 pylint-dev/pylint#5832 My suggestion for now is just mark thelint-3.10
build as not required.Checklist
RELEASE.md
file