Closed MuellerSeb closed 2 years ago
Hey @MuellerSeb,
wow what an amazing pull request. There was obviously a lot of clean-up necessary. In general, I think this pull request is excellent and will bring a lot of benefits. I like the new structure, testing features, and automatized version control.
The only thing I would like to change back is to transfer the tutorial with the installation of the package. May not be the most elegant way, but as there are not so many files, I think it is reasonable.
Regarding the ToDos, I can comment on the following:
add test.pypi.org account to check PyPI upload for all pushes to master I added the missing Tokens in .github/workflows/main.yml and hope this is what you meant?
coverage is not yet sent to coveralls to not produce an error I also added this Token, so I think we could also push to coveralls, can't we?
decide: should black and isort be applied? Yes, I think that would be good
decide: should hymod executables be part of the sdist and wheel? Yes, would be good, also applies to the tutorial files
decide: should the cli be installed as a script? I guess so. Are there other options?
before merging: remove "refactor_setup" from CI (was added for testing here) Ok, will do so, once we merge
Hey @MuellerSeb, wow what an amazing pull request. There was obviously a lot of clean-up necessary. In general, I think this pull request is excellent and will bring a lot of benefits. I like the new structure, testing features, and automatized version control. The only thing I would like to change back is to transfer the tutorial with the installation of the package. May not be the most elegant way, but as there are not so many files, I think it is reasonable.
I would still argue against putting them into the spotpy namespace. If they are under the src/spotpy
folder, these tutorial scripts could be imported, but that would just execute them which doesn't make sense in contrast to the example setups, which should be there (as I did it). My solution would be to at least include the tutorials in the source distribution by adding the folder to the MANIFEST.in
, since there it makes sense. When installing spotpy having the tutorials in the namespace it is also not that easy getting to the scripts, since they are buried in the installation and system dependent python package location.
Regarding the ToDos, I can comment on the following:
add test.pypi.org account to check PyPI upload for all pushes to master I added the missing Tokens in .github/workflows/main.yml and hope this is what you meant?
Cool!
coverage is not yet sent to coveralls to not produce an error I also added this Token, so I think we could also push to coveralls, can't we?
Also cool!
decide: should black and isort be applied? Yes, I think that would be good
Will do so!
decide: should hymod executables be part of the sdist and wheel? Yes, would be good, also applies to the tutorial files
Check!
decide: should the cli be installed as a script? I guess so. Are there other options?
I would shift that to a later pull request, since that would make click
a hard dependency.
before merging: remove "refactor_setup" from CI (was added for testing here) Ok, will do so, once we merge
I can also reset that short before merging.
I would still argue against putting them into the spotpy namespace.
You are right, separating the spot_setup example from the tutorials makes sense. With your restructuring, it is easy to just plug and play the tutorials and load the examples from the source code. It also makes sense, as at some point in the future I planed to make jupyter notebooks out of these tutorials, which also do not belong into the source code. Thank you for this suggestion!
decide: should the cli be installed as a script? I guess so. Are there other options? I would shift that to a later pull request, since that would make click a hard dependency.
Ok, let's wait with this one and push this pull request first
So good to see all the new tests and features! Is there anything missing for the coveralls testing at this point?
@thouska I think we are almost ready. If I add the coveralls upload and prepare the CI script for the merge, the CI on my side will not run. But we should that then test later on in this repository.
I think this PR is ready. Other stuff should be done later on in separate PRs:
One last question: Can I add myself to the author list? :wink:
@thouska added the missing github token env var.
should hymod be compiled with cython? (got some experience with that)
I think hymod.py is currently compiled with jit, so I wouldn't expect too much more speed potential with cython, but feel free to surprise me :)
update CHANGELOG.md
I will do that, once I tag
the merged files. Super curious to see, whether this will automatically result in a new pypi version. That would be pure magic :D
update CONTRIBUTING.md
I guess you have something in mind there?
add GUI script as command during installation
Spontaneously, I wouldn't know how to do that
One last question: Can I add myself to the author list? 😉
Yes, you can :)
should hymod be compiled with cython? (got some experience with that)
I think hymod.py is currently compiled with jit, so I wouldn't expect too much more speed potential with cython, but feel free to surprise me :)
Ok. I am just wondering about the unix version (I guess the exe version is working on most windows versions). There is a makefile to compile the cython extension. Would it maybe make sense to put hymod into a separate python package? I don't think it is a good idea to have a makefile in the source of spotpy.
update CHANGELOG.md
I will do that, once I
tag
the merged files. Super curious to see, whether this will automatically result in a new pypi version. That would be pure magic :D
I always update the changelog as the last thing before the tag, so the correct changelog is part of the release. Also, a develop version should be sent to test.pypi for each push to master.
It could a good idea, to create a release candidate as a pre-release with v1.6.0-rc1
(this works well with pypi, since it recognizes these versions as pre-release and pip install spotpy
will not use them (but could be set with a command line argument), see here for an example with gstools from this tag)
As just written, I would go to 1.6.0
after this merge, since py<3.7 was dropped.
update CONTRIBUTING.md
I guess you have something in mind there?
Update info about black
and isort
. Also it could be good to implement pylint
in the future and add info about it there as well.
add GUI script as command during installation
Spontaneously, I wouldn't know how to do that
No problem. This is rather easy.
One last question: Can I add myself to the author list? wink
Yes, you can :)
Whoop whoop. :tada:
This can be merged now. Everything else could be cleaned up later.
@MuellerSeb Thank you from my side also. Sorry for not responding earlier. What would you and @thouska think of moving hymod (and especially c-hymod) to another repo and make it an optional dependency of spotpy? That way, building spotpy from source would not require a compiler and deployment of new versions is simpler (no need for a binary release for each Python version, and the manylinux magic is not necessary).
@MuellerSeb Thank you from my side also. Sorry for not responding earlier. What would you and @thouska think of moving hymod (and especially c-hymod) to another repo and make it an optional dependency of spotpy? That way, building spotpy from source would not require a compiler and deployment of new versions is simpler (no need for a binary release for each Python version, and the manylinux magic is not necessary).
That was my thought as well. in GSTools for example, we use wheels to distribute compiled extensions, so there is also no need to have a compiler at least for the most common systems and python versions (MacOS (x64 and arm), Windows (32bit+64bit), Linux (32bit+64bit) for py3.7 - py3.10. Since it is only a cython file, this is very easy to do and the compilation could be done in the CI using cibuildwheel.
What is the difference between hymod_unix
, hymod_exe
and hymod_python
? (Maybe we could discuss this in an issue)
All right, let's bring this good stuff to the master branch. Thousand thanks again for your contribution @MuellerSeb and your feedback @abravalheri! All remaining tasks that popped up during the pull request can be followed up on in #288
CHANGES
setuptools>=62
and Python >= 3.7_version.py
when building sdist and wheelsrc/
based package structure for easier testing and so setuptools finds the correct package datablack .
andisort .
not yet applied to keep changes low)TODOs
"refactor_setup"
from CI (was added for testing here)v1.6.0
orv1.6.0-rc1
NOTES
Since I introduced
setuptools_scm
to derive the version, there is no need to put the version by hand anywhere.Pros
_version.py
will be generated automatically when installing with pip (even editable), or when building sdist / wheel1.5.17.dev18
)v
in tagmajor.minor.patch
: i.e.v1.5.18
v1.5.18-beta
,v1.5.18-rc1
Cons
0.0.0.dev0
(see here for a solution, but IMHO it's a corner case that should be discouraged)