Skip to content

CMake build from pip - #16

Merged
shimwell merged 41 commits into
open-radiation-sources:developfrom
DanShort12:cmake-build-from-pip
Dec 10, 2020
Merged

CMake build from pip#16
shimwell merged 41 commits into
open-radiation-sources:developfrom
DanShort12:cmake-build-from-pip

Conversation

@DanShort12

@DanShort12 DanShort12 commented Nov 6, 2020

Copy link
Copy Markdown
Contributor

This PR addresses #13 by implementing the following changes:

  • Packaging method is moved from binary wheel to source. This allows cmake to be configured appropriately for building the package when a user runs pip install.
  • Reorganisation of the repo. As the repo now needs to distribute CMakeLists.txt, the packaging is simplified by moving the source files into a src directory and creating a CMakeLists.txt within the package. This allows the package to be built either locally by envoking cmake, locally by running pip install . or python setup.py install, or by remotely sourcing the package via pip install parametric-plasma-source.
  • Support for cmake outside of git repo. The original build method relied on submodules to get dependencies. We now use find_package if we're outside of the repo, which allows setuptools to build the package in a temp directory.
  • PEP517-style dependencies. The recent PyPA change under PEP517 means that it is now possible to add some level of dependency management in a build (finally!). We make use of that via the build package and a pyproject.toml file to ensure the necessary dependencies are in place before we start the build. This in turn lets us install cmake and pybind11 as Python packages and use them in our package build to ensure they exist on the user's system.
  • Manual version setting. When building as a wheel we were able to provide some level of automation for setting the version by dynamically determining it from the latest version on PyPI. This isn't possible when the build is local, so the version now has to be manually bumped before releasing a new version.

Closes #13

@DanShort12

Copy link
Copy Markdown
Contributor Author

I've uploaded the successful build to test.pypi.org and it can be downloaded and installed (with corresponding dependency detection) by doing:

pip install "parametric-plasma-source>=0.0.9.dev0" --no-cache-dir --index https://test.pypi.org/simple --extra-index-url https://pypi.org/simple

I've run python build_and_run_model.py in an independent directory from my git repo and this now completes successfully.

@shimwell

shimwell commented Nov 6, 2020

Copy link
Copy Markdown
Contributor

So this looks like a nice improvement, it simplifies a few areas so I'm keen to merge it to develop and get a few testers to feed back.

@DanShort12

Copy link
Copy Markdown
Contributor Author

The version is now managed in parametric_plasma_source/__init__.py under the __version__ module variable. This gets read by setup.py to determine the version for the build, but also makes it accessible for anyone who wants to check the version in their code e.g.

import parametric_plasma_source as pps
print(pps.__version__)

This needs to be updated at a minimum before new packages are published to pypi or test.pypi. Packages are now published automatically to test.pypi when code is pushed to main, and packages are published to pypi automatically when the repo is tagged by creating a release in GitHub. This means that when a new version is to be released, the __version__ value should be updated in develop, changes merged into main (giving the potential for a final pre-release check by downloading from test.pypi), and then a new release created, which will release the new version to pypi.

Perhaps I can create a CONTRIBUTING.md and summarise in there.

@shimwell

shimwell commented Nov 6, 2020

Copy link
Copy Markdown
Contributor

I've uploaded the successful build to test.pypi.org and it can be downloaded and installed (with corresponding dependency detection) by doing:

pip install "parametric-plasma-source>=0.0.9.dev0" --no-cache-dir --index https://test.pypi.org/simple --extra-index-url https://pypi.org/simple

I've run python build_and_run_model.py in an independent directory from my git repo and this now completes successfully.

I can confirm this installs and works during transport. Also the pps.version helps

@shimwell shimwell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding that version info to the code on conduct. For anyone reading this, Dan an I have chatted offline about this PR

@DanShort12

Copy link
Copy Markdown
Contributor Author

Latest updates should resolve build issues reported under #17.

New name for Source abstract base class.
Make sample method (and methods that it calls) const.
Set library path and parameters as strings and pass into
CustomSourceWrapper.
Set external source via CustomSourceWrapper.
Remove pugixml as a dependency.
Also provide a function to call it via Python.
Check that we can sample from a source via OpenMC.
@DanShort12

Copy link
Copy Markdown
Contributor Author

Latest commits introduce tests for the OpenMC integration, which make use of the source_generator executable by including it into the Python package and using that to check we can sample the source via OpenMC. This should give better coverage and should mitigate issues with the OpenMC integration. This has meant that the OpenMC upgrade changes described under #18 are also included in this PR.

@DanShort12

Copy link
Copy Markdown
Contributor Author

Latest version includes tests for the cmake-only build, using the header-only version of Catch2. This runs in a separate cmake_build_and_test CI workflow from the Python package.

@shimwell shimwell mentioned this pull request Nov 29, 2020
@shimwell

Copy link
Copy Markdown
Contributor

Lots going on in this PR, tests pass and it looks like it is ready to merge if you are both happy @makeclean and @DanShort12 . Just wondering if #19 needs merging first?

@shimwell

shimwell commented Dec 7, 2020

Copy link
Copy Markdown
Contributor

As this is just going into develop and not main, I keen to merge it and let users try the pypi test install that will generated by merging to develop. This should provide some useful user feedback prior without harming the main branch.

I can merge this tomorrow midday if there are no objections

@DanShort12

Copy link
Copy Markdown
Contributor Author

Sounds good to me. @makeclean requested a few tweaks to add tests when building outside of PyPI, which I believe I've addressed.

@shimwell
shimwell merged commit a476233 into open-radiation-sources:develop Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants