Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to handle introduction of Python extension support #40

Closed
speth opened this issue Sep 11, 2022 · 8 comments
Closed

Update to handle introduction of Python extension support #40

speth opened this issue Sep 11, 2022 · 8 comments

Comments

@speth
Copy link
Member

speth commented Sep 11, 2022

The conda recipes need some updates to handle the addition of support for calling user-defined Python extensions from C++ (see Cantera/cantera#1382).

Currently, in the conda recipe, the shared library is built with the Python module disabled, but that is now also what determines whether support for Python-based extensions is compiled, since the requirements for Python extension support are the same as those for the Python module itself, and using the Python extension support requires the Python module to be present in any case. Further, libcantera now depends on a specific version of libpython, so we can't provide just one version of libcantera for all Python versions.

As discussed in Cantera/cantera#1382, it might make sense to just combine the libcantera package with the Python module, since the requirements for the two are now identical, and the first can only be fully used if the second is installed.

@ischoegl
Copy link
Member

ischoegl commented Sep 11, 2022

@speth ... thanks for creating this issue!

Before going to the step of eliminating libcantera (#41), I'd like to probe whether there are any alternatives that keep the existing layout, where libcantera packages the core, cantera packages the Python API, and libcantera-devel adds C++ & Fortran development interface.

  1. Is there a way to just throw an error if a user installs libcantera, but not cantera (example: libcantera-devel)? One way to go about this is to have rudimentary support for python_package=minimal (which handles throwing the error if the Python API is not found).
  2. How will this be handled once we add additional language extensions, e.g. Julia or .NET? I don't think that the solution should be to just add all API's to a single cantera package?

@bryanwweber
Copy link
Member

bryanwweber commented Sep 12, 2022

Further, libcantera now depends on a specific version of libpython, so we can't provide just one version of libcantera for all Python versions.

This is somewhat disappointing, because it means that we have to totally recompile the library for every version of Python we support, right? That's going to be a significant increase of CI resources. Is there any way to decouple them?

On the other hand, Conda-forge just has one job for each build in the matrix, and they all run in parallel, so at least over there, we aren't getting the benefit of the decoupling.

I suspect the easiest, least-time-consuming way to support this is to drop the libcantera package as a standalone. Then, we can have 3 recipes:

  1. libcantera-devel which provides C++ & Fortran, but not Python
  2. cantera which provides Python
  3. cantera-matlab which provides Matlab (depending on how we handle Su Sun's changes).

Is that feasible, or am I misunderstanding what the problem is, especially around libcantera-devel? I guess one concern would be incompatibilities between libcantera_shared.so in the libcantera-devel package and the same file in the cantera package. Possibly we can go back to statically linking the cantera package?

1. Is there a way to just throw an error if a user installs libcantera, but not cantera (example: libcantera-devel)? One way to go about this is to have rudimentary support for python_package=minimal (which handles throwing the error if the Python API is not found).

If we eliminate libcantera as a standalone, this will be solved I think... Installing libcantera-devel doesn't get you any Python support, so you'd get a ModuleNotFoundError if you tried to import it. Going the other way, (calling Python from C++) you'd probably get a compile-time error if libcantera-devel doesn't support Python functions, right?

2. How will this be handled once we add additional language extensions, e.g. Julia or .NET? I don't think that the solution should be to just add all API's to a single cantera package?

I think if these packages require Python to work, then there's no reason not to package them all up together, especially if it makes maintenance easier...

@ischoegl
Copy link
Member

ischoegl commented Sep 12, 2022

  1. I think if we attach F90 modules to cantera (where they apply, i.e. Linux only), then libcantera-devel will be just text files (headers, samples and a couple of scripts), and there won’t be compatibility issues?

@speth
Copy link
Member Author

speth commented Sep 21, 2022

  • Is there a way to just throw an error if a user installs libcantera, but not cantera (example: libcantera-devel)? One way to go about this is to have rudimentary support for python_package=minimal (which handles throwing the error if the Python API is not found).

  • How will this be handled once we add additional language extensions, e.g. Julia or .NET? I don't think that the solution should be to just add all API's to a single cantera package?

What you're asking for here is significantly more complicated than what I have currently implemented. Right now, we don't rely dynamically searching for libraries at runtime -- the linkage to the Python library has to be worked out at compile time, even if we're linking to a dynamic/shared library.

While this is technically possible (it is, after all, what Python has to do to load compiled modules), it's not something I have experience with, and I suspect that there is a lot of platform-specific mumbo jumbo. I had sort of been hoping to save dealing with this complexity for later, when implementing extension support for a different language.

This is somewhat disappointing, because it means that we have to totally recompile the library for every version of Python we support, right? That's going to be a significant increase of CI resources. Is there any way to decouple them?

No, you only need to recompile two files and relink to the correct Python library. The added CI time should be negligible (assuming it's done correctly).

I suspect the easiest, least-time-consuming way to support this is to drop the libcantera package as a standalone. Then, we can have 3 recipes:

1. `libcantera-devel` which provides C++ & Fortran, but not Python

2. `cantera` which provides Python

3. `cantera-matlab` which provides Matlab (depending on how we handle Su Sun's changes).

If the libcantera-devel package is built without any Python support, then you lose the ability to use the new Python extension capability from non-Python code. So the most functional version of this would require different versions of libcantera-devel for each supported Python version.

@ischoegl
Copy link
Member

ischoegl commented Sep 22, 2022

@speth and @bryanwweber ... given the circumstances, combining compiled code into a single package appears to be a reasonable approach. Caveat: there's no good solution for cantera-matlab, so it needs to remain separate.

Regarding libcantera-devel, I believe that moving the F90 modules into cantera would mean that what remains would be text files only (see my earlier comment above). Not sure that libcantera-devel would still be the best name, as there isn't a libcantera any longer.

@ischoegl
Copy link
Member

@speth ... thanks for looking into fixing the broken runners. Now that the latest action passes (:tada:), what are your thoughts here? Since the last post, Cantera became dynamically linkable (thanks again for the heavy lifting on that one), so did any of what was mentioned above change?

@speth
Copy link
Member Author

speth commented Feb 22, 2023

That is what I am working on figuring out. My expectation is that the changes in this PR will not be necessary due to the work on shared library support. Before closing this, though, I want to test the resulting packages on a couple of different machines to make sure they actually work.

@speth
Copy link
Member Author

speth commented Feb 25, 2023

The improved way of handling Python extensibility using Boost.DLL renders these changes unnecessary.

@speth speth closed this as completed Feb 25, 2023
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 a pull request may close this issue.

3 participants