python 3.10 suppport #287
python 3.10 suppport #287
Conversation
Added minimal build system spec pyproject.toml https://peps.python.org/pep-0518/ Docker files for testing insatllation procedure within virgin system Insert check in setup.py if python version >= 3.10 the extensions are freshly built
|
upps somehow the tests are not running -.- |
|
According to this: https://nose.readthedocs.io/en/latest/ nose2 or pytest may be a sufficient replacement. |
|
As supporting python 3.10 seems to have more implications as expected i converted the pull request to a draft. Refer #288 for some findings |
|
@Thaulino Thanks for working on Python 3.10 support! I'll take a look at the test failures and see if we can get this working. |
|
To support python 3.10, we have to switch from nose to pytest. |
|
My mistake. It looks like you are already doing that. Nose is causing the test failures though. It looks like nose is still referenced in |
|
@michaelbynum thanks for your review, indeed i am replacing nose with pytest allready, i am Just partly finished but the quicktest allready works ✌️ |
|
That's great! As @kaklise said, thanks for the contribution! |
…heels for different python version and platforms
… os for coverage combine to ubuntu (speed up)
|
Hi everyone, i refactored the workflows and more for a better readability, a, from my point of view, more phytonic way of distributing the package and of course python 3.10 support. Regarding the more phytonic way of distributing the package I think it is more phytonic to provide wheels with prebuilt extensions for most of the platforms and python versions plus a source distribution for everyone who must or wants to build the package local. If we do so, it is important to enable the built flag in the setup.py file. To sum up my suggestion:
This way it is granted that if the installation via pip succeeds the wntr package is working. kind regards |
|
@Thaulino Thanks for continuing to work on this. |
|
hi guys, kind regards |
|
@Thaulino Thanks for checking in. We plan to review the PR next week. |
There was a problem hiding this comment.
Hi @Thaulino
We've looked over this and I do have a couple comments. We would prefer that the docker files not be included in the commit, since they are only used for local development as opposed to use in the actions.
The other thing is that we do need to keep the _evaluator and _network_isolation binaries included in the repository. These almost never change, other than being updated for new versions of Python (they haven't changed since before 3.9 was a thing, maybe since before 3.8, because I remember having to get them built to be added in).
We are aware that this technically isn't best practice, but we have a use case where people are using WNTR in development mode but do not have access to C++ compilers installed on their machines. So they pull in or make changes to Python files elsewhere in the package, but can't build the aml/network_isolation plugins themselves, so we need them in the repository.
With your updates here, we would then take the artifacts of the build and add in py310 support as well once we pull in the PR. If we were just doing PyPI, we could do just wheel upload, but our use cases don't allow for that. Right now we are thinking of putting in a flag, like what you had for the 3.10 in setup.py, that would only set build to true in the GitHub workflow, and be false by default when pulling from the repository. Hopefully that all makes sense.
I think if you can exclude the dockerfile pieces and undelete the binaries, we would be ready to pull this in. I am currently going through the test_examples to try to figure out why they aren't working, so that we don't have to exclude them, but I don't think we need to wait for that :-)
@kaklise anything else?
|
Please ignore the last 2 commits, i used the wrong branch -> reverted all changes |
|
@dbhart thanks for the the feedback and the detailed explanation. I have excluded the docker files and inlcuded the binaries again - as a result we should be ready for a merge. kind regards |
|
hmm, that is strange i restarted the workflow on my fork (same hash) and it succeeded. |
…tests as it should be quickt
|
Renamed the test marker: time_consuming (because the marked tests are time consuming) |
|
Hooray, all tests succeeded but i do not understand why this one does not succeded: |
|
Hi @Thaulino , I am working on going through all of it! Thanks!
|
Good day everyone,
What i have done:
Added minimal build system spec pyproject.toml -> https://peps.python.org/pep-0518/ -> this will also install numpy before running setup.py
Insert a check in setup.py if python version >= 3.10, if true the extensions are freshly built
As i wanted to test if this works on a virgin system, i added a docker file for testing installation procedure within virgin ubuntu system.
Will solve #254
Looking forward for a review
kind regards