Skip to content

Make MCPL a Runtime Optional Dependency #3429

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

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

ahnaf-tahmid-chowdhury
Copy link
Contributor

Description

This pull request refactors the MCPL interface in OpenMC to make MCPL a runtime optional dependency rather than a build-time one. This version enhances the previous effort by implementing cross-platform dynamic library loading and discovery mechanisms, including support for Windows.

Benefits

  • Simplified Build Process: OpenMC can be built without requiring MCPL to be pre-installed.
  • Reduced Build-Time Dependencies: Contributes to a leaner core build.
  • Flexibility: Users can install MCPL separately. OpenMC will attempt to load it at runtime using platform-appropriate methods.

Key Changes

  1. CMake Modifications:

    • Removed the OPENMC_USE_MCPL CMake option.
  2. Cross-Platform Runtime Library Loading (mcpl_interface.cpp):

    • The MCPL shared library is now loaded at runtime using:
      • dlopen() on POSIX-like systems (Linux, macOS).
      • LoadLibraryA() on Windows.
    • Pointers to required MCPL API functions are resolved using:
      • dlsym() on POSIX.
      • GetProcAddress() on Windows.
    • Platform-specific library handles (void* or HMODULE) and cleanup functions (dlclose or FreeLibrary) are used.
    • An McplApi struct holds these function pointers.
    • A one-time initialization function (initialize_mcpl_interface_if_needed) handles the loading and symbol resolution.
  3. Library Discovery Mechanisms:
    The initialize_mcpl_interface_impl function attempts to locate the MCPL library in the following order:

    1. mcpl-config --show libpath Command:
      • Uses popen() on POSIX systems.
      • Uses _popen() on Windows systems (requires mcpl-config.bat or .exe to be in the system PATH).
      • The output path is used to load the library.
    2. Standard Library Names:
      • Tries libmcpl.so, libmcpl.dylib on POSIX.
      • Tries mcpl.dll, libmcpl.dll on Windows. These are searched in standard system library locations (e.g., those covered by PATH on Windows or LD_LIBRARY_PATH on Linux).
  4. Avoidance of mcpl.h at Compile Time:

    • mcpl.h is not included to ensure MCPL is not a compile-time dependency.

    • Necessary MCPL data structures (e.g., mcpl_particle_t) and function signatures are manually re-declared within mcpl_interface.cpp as mcpl_particle_repr_t, etc.

      Important Note: These manual declarations must precisely match the ABI of the MCPL library (MCPL 2.2.0 is currently targeted). pragma pack(1) is used for structure packing consistency.

  5. Error Handling and User Guidance:

    • If the MCPL library cannot be found or loaded, OpenMC issues a warning (once, by master MPI rank) detailing the attempted load methods and errors.
    • If an MCPL-dependent feature is used and the library is unavailable, OpenMC fatal_errors with an informative message, suggesting installation (e.g., pip install mcpl) and how to make the library findable.
    • The is_mcpl_interface_available() function can be used to programmatically check MCPL availability.
  6. Code Updates:

    • All calls to MCPL functions within mcpl_interface.cpp use the dynamically resolved function pointers.
    • The MCPL_ENABLED preprocessor logic has been removed.

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

@tkittel Does this PR look sensible to you? From a quick scan, it looks very similar to what we did with the NCrystal interface.

@shimwell
Copy link
Member

This PR would be useful for getting the sci kit build core working again. Any objections to merging

Copy link
Member

@shimwell shimwell left a comment

Choose a reason for hiding this comment

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

Thanks Ahnaf another nice build system improvement.

LGTM

I will merge this in on Friday if there are no objections

@shimwell shimwell added the Merging Soon PR will be merged in < 24 hrs if no further comments are made. label Jul 2, 2025
Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

Just putting a placeholder review as I'd like to take a look before we merge this. I was also hoping to get @tkittel's vote of confidence on this. I'm assuming he's been in the loop on this?

@shimwell
Copy link
Member

shimwell commented Jul 3, 2025

I'm assuming he's been in the loop on this?

I've not seen any conversations on this PR outside of this PR thread.

@tkittel
Copy link
Contributor

tkittel commented Jul 3, 2025

Hi guys. Sorry, I am on vacation so I didnt have a chance to look at this. I like the aim of this PR though :-)

Not 100% sure when I might have a chance to have a look at the details, but perhaps I can find a quiet moment within the next few days.

@shimwell shimwell removed the Merging Soon PR will be merged in < 24 hrs if no further comments are made. label Jul 4, 2025
@tkittel
Copy link
Contributor

tkittel commented Jul 4, 2025

Hi guys,

So I had a very quick skim through, and I have a few comments. Do keep in mind though, that the OpenMC-MCPL bindings were initially contributed by @ebknudsen not me.

In general, big thumbs up to the work here. And yes, the intention is to have the MCPL C ABI stable enough to do these kinds of things. I have a few minor issues though:

  1. I would feel more comfortable (and I think it would be more easy to understand going forward) if the various structures from mcpl.h were replicated exactly. I understand that the memory layout of for instance:

    typedef struct MCPL_API { void * internal; } mcpl_file_t;  

    is in reality the same as the raw void-pointer, and that it therefore technically works to pretend it is instead:

    typedef void* mcpl_file_handle_repr_t;

    Would it not be more obvious to simply have a section where you repeat the necessary typedef's from mcpl.h in your code, with clear comments around it telling people that these must be kept EXACTLY as in mcpl.h?
    Anyway, it is not a deal-breaker for me.

  2. As was also discussed during the PR of the corresponding NCrystal feature, I am very much skeptical of having dlclose/FreeLibrary calls. Unlike free'ing a piece of allocated memory like a good citizen, there is simply too many potential pitfalls when it comes to library unloading.

  3. Instead of having the diagnostic message just telling people that they might want to "pip install mcpl", perhaps it would be sensible to also remind them of the option to conda install mcpl? (to avoid getting pypi pkgs in the users conda env, where conda mcpl pkgs would be the better choice.

  4. If I understand correctly, you now emit a fatal error if you run on an mcpl file containing a particle type not supported by OpenMC. That is not in general how we recommend applications to consume MCPL files, since it makes it harder to interchange files between applications. Instead we recommend that any unsupported particles encountered are skipped over, and that the application emits some sort of diagnostics message to warn the user (this could perhaps be a final summary message or warning like: "3% of the particles in the MCPL source were ignored since their particle type is not supported by OpenMC").

    As an example, Providing an MCPL file to McStas which contains both neutrons and gammas from a moderator simulation, McStas will use the neutrons and merely warn the user that so and so many particles were ignored.

    The alternative is that users will have to pre-filter MCPL files depending on the application in which they want them to be used, and at that point they become a bit less "universally interchangeable".

Anyway, apart from the points above and the fact that it was just a quick skim-through, it all looks great to me :-)

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.

4 participants