-
Notifications
You must be signed in to change notification settings - Fork 25
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
Updated cadCAD compatibility mode and tests to be compatible with latest cadCAD v0.5.3 #68
Conversation
@smngvlkz Thanks for the fix! I have some suggestions I'll make in the PR review. @imDarshanGK Please don't come here to mine GitHub activity 🙏 |
pyproject.toml
Outdated
@@ -8,10 +8,10 @@ packages = [ | |||
] | |||
|
|||
[tool.poetry.dependencies] | |||
python = ">=3.8,<4.0" | |||
python = ">=3.9,<4.0" |
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.
@smngvlkz I imagine you removed 3.8 because cadCAD says it only supports >=3.9? The test for 3.8 still passes in the CI pipeline (see GitHub action at .github/workflows/python.yml), so we should be able to leave this as >=3.8.
If you'd like to support further, you can add Python 3.11 and 3.12 to the CI pipeline and see if the tests pass (if they fail, remove them again), and then set the Python version compatibility appropriately here—<4.0 was probably too open; for example, if they add 3.13 at some point, we can't assume that radCAD or its dependencies will support 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.
The radCAD version should be updated in this file and radcad/__init__.yml
to v0.14.0, and an entry should be added in the changelog for the new version explaining the changes.
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.
@BenSchZA Thank you for your review. I believe I have addressed all the suggested changes, including updating the cadCAD dependency, expanding Python version compatibility, and refining the project configuration. I have also updated the CHANNELOG.md file to reflect the changes. Please let me know if this revised PR meets your requirements and if not, please let me know so I can make the necessary changes
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.
Also to ensure we are future-proof and aligned with the dependencies requirements, it is wise to set the minimum Python version to 3.9. So I have decided not to change it back to 3.8
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.
Thanks @smngvlkz!
Also to ensure we are future-proof and aligned with the dependencies requirements, it is wise to set the minimum Python version to 3.9. So I have decided not to change it back to 3.8
Generally best to judge support for Python versions based on your own package tests—it's not possible to go through every dependency and confirm that Python 3.8 is/isn't supported. Would prefer to maintain support for 3.8 for as long as possible.
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.
i.e. if the CI test for 3.8 doesn't pass, then we can remove it, else let's leave as is.
CHANGELOG.md
Outdated
|
||
- Resolved comp atibility issues with Python 3.10 | ||
|
||
### Maintenance |
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.
This should be included under "Changed" or "Added". "Maintenance" isn't a heading in the Keep a Changelog standard I don't think.
CHANGELOG.md
Outdated
- Updated CI pipeline to test against Python 3.11 and 3.12 | ||
- Refined Python version compatibilit y range in project configuration | ||
|
||
## [0.13. 0] - 2023-10-26 |
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.
Some formatting issues here and above it seems—see spaces.
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.
Formatting issues fixed
pyproject.toml
Outdated
@@ -8,7 +8,7 @@ packages = [ | |||
] | |||
|
|||
[tool.poetry.dependencies] | |||
python = ">=3.8,<3.13" | |||
python = ">=3.9,<3.13" |
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.
I see in this latest commit that you removed support for 3.8 again. Is there a specific reason for this? If it is not supported anymore, it should be indicated in the changelog under "Removed".
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.
Would suggest setting version to <=3.12
instead of <3.13
to be more explicit, e.g. in case a version 3.125 is introduced (which although unlikely, is possible).
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.
I just responded on the above comment for this
@smngvlkz You can see that in the above CI pipeline, Python 3.11 fails. It's possible there is no easy fix for this and that only Python versions 3.8 to 3.10 should be supported. |
Yeah, but let me try to see if I can find a way to fix it. If all fails then we will leave it to versions 3.8 - 3.10 |
.github/workflows/python.yml
Outdated
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
python-version: [3.8, 3.9, '3.10', '3.11', '3.12'] | ||
python-version: [3.9, 3.10, 3.11, 3.12] |
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.
The quotes should be included, otherwise it's interpreted at 3.1
.
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.
I have made the necessary changes. The CI pipeline should pass now
@smngvlkz I believe Python 3.11 is failing because Pytest needs to be updated to v7.2.0. You can see this by looking at the CI logs and seeing that Pytest throws an error Let's also add back Python 3.8 to the CI pipeline to check if it fails, before removing support for Python 3.8 as suggested. |
pyproject.toml
Outdated
@@ -9,6 +9,7 @@ packages = [ | |||
|
|||
[tool.poetry.dependencies] | |||
python = ">=3.9,<3.13" | |||
memory_profiler = "^0.58.0" |
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.
@smngvlkz This shouldn't be a core dependency—it's only used when performing benchmarking.
I see the test is still failing. @smngvlkz I'd suggest we remove support for 3.11 and 3.12 from the scope of this PR and instead make the necessary updates to indicate support for Python 3.8-3.10 as it was before. I think supporting Python 3.11+ will take some work. |
This PR resolves issue #67. |
652505f
to
73a1c68
Compare
@BenSchZA I have reset the branch to the starting point |
@BenSchZA Please review the output from running
The latest version of cadCAD no longer supports Python 3.8. To leverage the benefits of Python 3.9, I had to remove support for Python 3.8 from our project. |
…nto fix/compatibility-mode
Changes Made:
Test results:
Channelog Entry
I believe this update resolves the compatibility issues with Python 3.10 while maintaining the existing functionality of radCAD