Add pypi package #1075
Add pypi package #1075
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1075 +/- ##
==========================================
+ Coverage 73.50% 73.52% +0.01%
==========================================
Files 365 365
Lines 48247 48243 -4
==========================================
+ Hits 35466 35470 +4
+ Misses 12781 12773 -8
Continue to review full report at Codecov.
|
bryanwweber
force-pushed
the
add-pypi-package
branch
from
7e3c2cf
to
01707ae
Compare
|
I should also note that modifying |
There was a problem hiding this comment.
Looks pretty reasonable to me, overall. I agree with the idea of moving the wheel builds to a separate repo where those builds only run after updates to select branches (main and 2.5). This might be a good opportunity to set up the conda builds to do something similar.
bryanwweber
force-pushed
the
add-pypi-package
branch
3 times, most recently
from
9a7692b
to
20a2156
Compare
| @@ -1896,12 +1911,10 @@ def postBuildMessage(target, source, env): | |||
| print("- To run the test suite, type 'scons test'.") | |||
| print("- To list available tests, type 'scons test-help'.") | |||
| if env['googletest'] == 'none': | |||
| print(" WARNING: You set the 'googletest' to 'none' and all it's tests will be skipped.") | |||
| print(" WARNING: You set the 'googletest' to 'none' and all its tests will be skipped.") | |||
There was a problem hiding this comment.
Should this be issued by logger? (Although the same applies to the surrounding lines, so replacing things just here would be odd.)
There was a problem hiding this comment.
Yes, ideally it should. I just couldn't help fixing this typo 🤪 I'm still hoping to eventually be able to refactor this file to use those features I added in buildutils.py. I think the changes from #1137 are a great step in the right direction too!
bryanwweber
force-pushed
the
add-pypi-package
branch
3 times, most recently
from
0c2d76b
to
94eb9d8
Compare
Use a dictionary to collect the license file paths and names. This data structure is more natural for this mapping of packages to files. It also simplifies the loop to collect the files.
Add a new option for the Python package to build a source distribution. It is added as a new interface to simplify copying files. All of the Cantera plus external library source code is copied into the interface folder for building. Actually building the source distribution is handled by the 'build' package, which becomes a new dependency to build the sdist. 'build' handles installing all the build-time dependencies in an isolated environment using specifications in pyproject.toml.
This is the same mechanism as employed for config.h in the main SConstruct. It's needed here to avoid repeatedly copying the include directory into the sdist, which fails due to the existing directory.
bryanwweber
force-pushed
the
add-pypi-package
branch
from
d478578
to
c5b5464
Compare
The function is not used anywhere within Cantera. When this function is removed, the CANTERA_ROOT define in config.h can be removed as well.
The sysconfig module is available since Python 3.2. Distutils is deprecated and slated for removal from Python in the next few years. Likewise, the "SO" config variable has been deprecated for some time and was slated for removal in Python 3.8, although that seems not to have happened.
The bug was fixed in Python 3.7.3, so it no longer required.
This change mirrors the one for the source distribution. This change results in a non-executable template file (setup.cfg.in), which makes edits easier. This change removes the requirement to template the Python extension into setup.py, and adds a Setuptools extension module. This results in correct builds when bdist_wheel is specified and uses package_data properly.
This change provides similar benefits as for the other two Python distributions. It should allow easier uploading to PyPI and now includes the README and License files.
For some versions of Python, the DLL module extension included in the sysconfig module is not correct. In that case, the file extension needs to be constructed manually. To make this information easier to get to, the JSON module is used to parse data returned by sysconfig from the Python which will be building the module.
bryanwweber
force-pushed
the
add-pypi-package
branch
from
3b53909
to
fb0826c
Compare
|
@speth / @ischoegl This is ready for another review. All the builds are passing now over here: https://github.com/bryanwweber/cantera/actions/runs/1505864912 Note that the last commit on this branch should be dropped before merging to |
There was a problem hiding this comment.
🎉 ... thanks for your work on this, @bryanwweber. As mentioned earlier, packaging is not necessarily something I'm familiar with, but the coding itself looks fine to me. The only broader question I have is whether the wheels get tested before shipping (i.e. unit tests). I guess that they should 'just work', but then you never know ... .
PS: I noticed that you started building for Python 3.10 ... would it make sense to add this to the regular CI?
Great question 😊 I just added testing of the wheels with these updates, the
Yeah, we definitely should. Do you want to add that in your Intel PR, or shall I add it here? |
Go ahead and add it here. The Intel PR is a little thorny ... while compilation works, I'm not sure why those tests are failing. |
I'll add a separate PR, to simplify review. |
There was a problem hiding this comment.
Thanks, @bryanwweber. I don't have any further comments on this. Do you want to drop the final commit now so this can be merged?
bryanwweber
force-pushed
the
add-pypi-package
branch
from
fb0826c
to
0e8791a
Compare
|
@speth All set, I guess we'll have to wait for the checks to run again, and I think you have to do another "approve" checkmark. |
Changes proposed in this pull request
python_sdistinterface, which builds a source distribution for Python. This sdist can be used to build all the platform wheels we care to supportcythonandpython_minimalinterfaces to use setup.cfg style configuration of the build and installIf applicable, fill in the issue number this pull request is fixing
Closes Cantera/enhancements#5 and #1114
If applicable, provide an example illustrating new features this pull request is introducing
In testing this pull request, I found that the CI jobs to build all the wheels take 4+ hours. This is partly due to compiling the complete library for every architecture/Python version that is supported. However, I think it is valuable to confirm that we can build from the source distribution. So once this PR is ready for merge, I propose that we create a separate repository that only houses the workflow configuration for building wheels, and runs a build whenever
cantera/mainis updated. That way, pull requests to this repository won't be held up by waiting for the wheels to build. Before merging this PR intocantera/mainwe should delete thepython-package.ymlworkflow configuration and move it to the new repository.The CI for this branch is running over on my fork: https://github.com/bryanwweber/cantera/actions/runs/1038707541
You can test the wheels for yourself by running:
Checklist
scons build&scons test) and unit tests address code coverageAside for new contributors: The branch that's attached to this pull request has been substantially cleaned up prior to proposing the pull request. Based on the history here, it looks like this feature came fully formed from my fingers into the computer. That is a total and complete fabrication. If you want to see the original history of this branch, go here: https://github.com/bryanwweber/cantera/commits/original-pypi-branch
The point is, it takes a lot of iteration to get complex features correct, but please make as many commits as you need! You can always clean it up later.