mutationpp / Mutationpp

The MUlticomponent Thermodynamic And Transport library for IONized gases in C++
GNU Lesser General Public License v3.0
103 stars 58 forks source link

Add Python bindings with pre-compiled wheels #169

Closed rdbisme closed 3 years ago

rdbisme commented 3 years ago

This PR is based on the amazing work of @BBArrosDias @ #155. I slightly refactored the code base, notably:

Things still to discuss:

You should be able to test the python pre-compiled wheel easily on Linux and Mac (I uploaded the wheels here on the test PyPI instance: https://test.pypi.org/project/mutationpp)

codecov[bot] commented 3 years ago

Codecov Report

Merging #169 (0d1a3e2) into master (bd5fae1) will increase coverage by 0.91%. The diff coverage is n/a.

:exclamation: Current head 0d1a3e2 differs from pull request most recent head 57fa1f6. Consider uploading reports for the commit 57fa1f6 to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           master     #169      +/-   ##
==========================================
+ Coverage   70.21%   71.13%   +0.91%     
==========================================
  Files         134      133       -1     
  Lines        8458     8961     +503     
==========================================
+ Hits         5939     6374     +435     
- Misses       2519     2587      +68     
Impacted Files Coverage Δ
src/apps/mppequil.cpp 26.81% <ø> (ø)
src/gsi/MassBlowingRateNull.cpp 0.00% <0.00%> (-16.67%) :arrow_down:
src/thermo/Nasa7Polynomial.h 78.57% <0.00%> (-12.34%) :arrow_down:
src/thermo/Composition.h 90.90% <0.00%> (-9.10%) :arrow_down:
src/transport/CapitelliIntegrals.h 55.55% <0.00%> (-5.99%) :arrow_down:
src/transport/CollisionIntegral.h 82.35% <0.00%> (-4.32%) :arrow_down:
src/transport/LangevinIntegrals.cpp 92.59% <0.00%> (-3.84%) :arrow_down:
src/transport/Transport.h 66.66% <0.00%> (-3.34%) :arrow_down:
src/transport/DiffusionMatrix.h 80.00% <0.00%> (-3.34%) :arrow_down:
src/gsi/GSIStoichiometryManager.h 46.66% <0.00%> (-3.34%) :arrow_down:
... and 114 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bd5fae1...57fa1f6. Read the comment docs.

rdbisme commented 3 years ago

PS: Most of the modifications are associated to me running clang-format on the codebase.

rdbisme commented 3 years ago

Before merging @jbscoggi let me know such that I squash commits into one.

jbscoggi commented 3 years ago

I will answer the two discussion questions here:

  1. I agree, the wheel should be self contained. With that said, do you think it would make sense/is it possible, to make the M++ a dependency which carries the data directory. So that when the wheel is installed, it also installs M++?
  2. @BBArrosDias and I discussed using the markdown files because it is easier to VC and read during code reviews. Would that be possible with your suggestion?
jbscoggi commented 3 years ago

Btw @rubendibattista, the pip install works great!

rdbisme commented 3 years ago

This seems good to me! My main comment is that in the future if you could breakup the changes into more concise PRs, this would be easier to review and to reason about. For example, I think this PR is at least 2 distinct PRs, 1) for fixing the python build and 2) reorganizing / formatting. It might sound like a bit of a nitpick, but I think it is easier to understand the reasoning behind different aspects of the PR if it is self contained without additional changes. Otherwise, great work as usual :)

You are absolutely right. I was a bit in a development trance and I did not keep track of things :). I'll try to be more rigorous and trying to not expand too much the PR

I will answer the two discussion questions here:

1. I agree, the wheel should be self contained.  With that said, do you think it would make sense/is it possible, to make the M++ a dependency which carries the data directory.  So that when the wheel is installed, it also installs M++?

2. @BBArrosDias and I discussed using the markdown files because it is easier to VC and read during code reviews.  Would that be possible with your suggestion?
  1. No problem to embed the data directory, but the logic in the code should be changed then. The default path of the data directory should be "hardcoded" at configure/compile time, allowing to expand it with the environment variable or a configure file. Such that the "default" data things are embedded, but people can add their own databases without fiddling with the install directory. Something like they put their db into (for Linux) .local/config/mutationpp/data and Mutationn will look for things in addition to the default directory. What do you think?

  2. What I do in josiepy is to store the notebooks as real notebooks in the git repository. They get converted into HTML at documentation generation, adding a "Binder" button to the top. If you click on the Binder button, you are transferred on an ephemeral instance of binder where you can run the notebook on your browser without the need of installing stuff. I think this is great! Keeping the notebooks in the repository as is allows to test them and be sure that the code in them is up to date. I'm pretty sure that if we keep the Markdown, it will go out of sync very soon!

I'm going to squash the commits then.

rdbisme commented 3 years ago

Also:

  1. at the moment the automatic upload of the wheels is on the test instance. Do you want me to switch it to the actual PyPI index? Or do we keep it like this, a bit overshadowed, for a while, to test it?
  2. in order for the automatic upload to work, I need to create the actual project on PyPI, getting the API token, and adding it into the repository secrets. I'll see if I can do it already, otherwise I'll let you know. I'll create the project on PyPI and add you also as maintainer. You need to register here: https://pypi.org/
jbscoggi commented 3 years ago

Let's keep it on the test instance for now. I will ask people to test it out in the developer group and to provide some feedback at the next mutants meeting. We can make a decision then. What do you think?

rdbisme commented 3 years ago

Ok, squashed the commits. @jbscoggi if you tag to v1.0.5 we can also test the upload on TestPyPI. I already added the corresponding secret that should allow that.

jbscoggi commented 3 years ago

Tag done!

jbscoggi commented 3 years ago

Btw, I should have realized this before, but I think we should have bumped to v1.1.0 since this is really adding a new feature.

rdbisme commented 3 years ago

Yeah, we can do this once we go on the official PyPI instance I'd say.

jbscoggi commented 3 years ago

OK, fair enough. Thanks @rubendibattista and @BBArrosDias, this is a really nice new feature!

BBArrosDias commented 3 years ago

Thanks, @rubendibattista, for optimizing the usage of the package. Indeed this will help a lot of people. I think that we can optimize some functions, but with this setup, it will be easier. Does the code coverage work fine?

rdbisme commented 3 years ago

The code coverage is untouched, the percentage it gives is on the C++ codebase, it does not take into account any Python code. The slight reduction is about the Python interfaces that are not tested by the C++ test suite.

I think the next steps would maybe be to rewrite the binary executables as Python scripts (more easily manageable). If we want coverage on the Python side, I need to check how coverage.py works for compiled extensions. I think it won't be of much use, we will add Python coverage if we will have a substantial Python codebase implementing actual logic on the Python side.

rdbisme commented 3 years ago

The upload of the wheels works as expected 🎉 https://test.pypi.org/manage/project/mutationpp/releases/

jbscoggi commented 3 years ago

Can you add me as a maintainer? I'm jbscoggi. Thanks.

BBArrosDias commented 3 years ago

Me also? barrosDias. Thanks