Initial Windows Support #2919

HunterBelanger · 2024-03-19T18:00:18Z

Description

This PR is an initial attempt to adding native Windows support to OpenMC. With these modifications, I have been able to compile vanilla OpenMC on Windows, using both MSVC (19.38) and the Intel compilers. I have made this a draft PR for the moment due to the following changes / restrictions.

I found that the version of xtensor we are using in the submodules has a memory bug on Windows. For the time being, I have simply commented out loading the xtensor submodule directories, and currently get more recent versions with FetchContent. ~~If we want to merge this, we will need update the submodule (or move to FetchContent ??)~~ (see Update xtl and xtensor submodules #2941).
In general, I was able to get most of the Python API to work, with the exception of the cythonized bits like resonance reconstruction and fast ENDF floating point conversion. For now, I have simply disabled them on Windows. Otherwise, it seems to work just fine ? I was able to run the pin cell depletion example problem from Python.
With Intel compilers, OpenMP parallelism works fine. With MSVC, an experimental flag needs to be added: /openmp:llvm. With this, MSVC builds it in parallel, and it seems to work. This experimental OMP support on Windows does not like the #pragma omp atomic write seq_cst we have in SharedArray, so I have added compilation guards to use a simple #pragma omp atomic write when using MSVC. In theory, this could open us up to a race condition here, but I haven't observed any problems with this yet (and in any case, using Intel avoid this problem).
Many of the tests are failing on Windows. I have not been able to figure out why this is, however, as I can barely even get the tests to pass on my linux machine half the time. The results which I have obtained thus far seem to be in agreement with what I would expect. I am not sure to what extent some tests might be failing due to differences in Python dependencies on Windows ?

I am very open to suggestions on how to tackle the remaining problems ! I am definitely not a Windows developer, and have yet to find/understand good tools to properly examine this on Windows. For example, it will almost always crash in Debug mode because we apparently do some bad things with iterators in some spots. If you compile in Release mode, these do not pose a problem. Please feel free to test it out on a Windows machine and find all the bugs I have probably missed ! For now, I am not building HDF5 myself, but simply download and install the pre-built binaries from the HDF5 website.

Fixes #1243

Checklist

I have performed a self-review of my own code
I have run clang-format (version 15) on any C++ source files (if applicable)
I have followed the style guidelines for Python source files (if applicable)
I have made corresponding changes to the documentation (if applicable)
I have added tests that prove my fix is effective or that my feature works (if applicable)

… version

openmc/deplete/pool.py

gridley · 2024-03-30T18:51:03Z

Nice! I always wondered why we couldn't do this. Pre-packing a binary for windows could be very nice for some users.

Co-authored-by: Gavin Ridley <gavin.keith.ridley@gmail.com>

paulromano · 2024-04-04T20:27:01Z

@HunterBelanger Regarding the xtl/xtensor update, can you submit a separate PR just updating our submodules to the latest released version of each? That way we can consider separately (and later) whether we want to adopt FetchContent rather than lump it with the Windows-related changes here.

HunterBelanger · 2024-04-04T21:52:50Z

Yeah, I can make a separate PR to update the submodules and discuss FetchContent. For the time being though, I figure we probably want to keep the temporary work around that I have in this branch, just so it functions, and we can update this branch when we finally want to merge ? Otherwise, this won't work for anyone trying to test it.

paulromano · 2024-04-05T07:21:55Z

@HunterBelanger Yes, once the submodule update is merged separately, we can update this branch to remove the FetchContent changes (I can help with that if needed).

paulromano

@HunterBelanger Thanks a lot for starting this! I made some updates on your branch. Please take a look and let me know if you have any objections or questions. See below for a few questions about which symbols are getting exported.

paulromano · 2024-08-16T12:57:24Z

openmc/deplete/pool.py

+# Not sure why, but using multiprocessing on Windows leads to many transport
+# simulations being run over eachother and leads to catastrophe.
+if sys.platform == 'win32':
+    USE_MULTIPROCESSING = False


We ought to get to the bottom of this since it should work in principle on Windows

I completely agree, though I have not had time to look at it since. It was a while ago when I examined this, but from what I could tell I think it is a Python/Windows problem and not necessarily a bug on our side.

Just as a note, turns out this same problem exists on my student's M1 mac.

include/openmc/message_passing.h

include/openmc/settings.h

include/openmc/tallies/filter.h

paulromano · 2024-08-16T13:59:40Z

src/error.cpp

@@ -30,7 +30,7 @@ int OPENMC_E_PHYSICS {-10};
 int OPENMC_E_WARNING {1};

 // Error message
-char openmc_err_msg[256];
+char OPENMC_API openmc_err_msg[256];


We shouldn't need the export in .cpp files, no?

This is what I thought too, originally, but from what I could tell, things only work if the export is in both the .hpp and the .cpp files. I can check again, but I remember struggling with this for a while before I figured out both were needed (I am definitely not a Windows developer 😅).

paulromano · 2024-08-16T14:00:07Z

src/message_passing.cpp

@@ -5,7 +5,7 @@ namespace mpi {

 int rank {0};
 int n_procs {1};
-bool master {true};
+bool OPENMC_API master {true};


Shouldn't need an export?

As for the header, icx wants this variable exported, and currently I think we need the export in both the hpp and the cpp.

HunterBelanger

Thanks for taking a look at this, @paulromano ! All of your name changes make sense to me ! One thing I also forgot to mention is that since the merging of random rays, this no longer compiles with MSVC (due to the OpenMP features that are used there), but it still compiles with icx for me. This leads to pains with the Intel DLLs and loading the Python library, but it does seem to work.

HunterBelanger · 2024-08-16T14:17:08Z

src/error.cpp

@@ -30,7 +30,7 @@ int OPENMC_E_PHYSICS {-10};
 int OPENMC_E_WARNING {1};

 // Error message
-char openmc_err_msg[256];
+char OPENMC_API openmc_err_msg[256];


This is what I thought too, originally, but from what I could tell, things only work if the export is in both the .hpp and the .cpp files. I can check again, but I remember struggling with this for a while before I figured out both were needed (I am definitely not a Windows developer 😅).

include/openmc/tallies/filter.h

include/openmc/message_passing.h

HunterBelanger · 2024-08-16T14:32:52Z

src/message_passing.cpp

@@ -5,7 +5,7 @@ namespace mpi {

 int rank {0};
 int n_procs {1};
-bool master {true};
+bool OPENMC_API master {true};


As for the header, icx wants this variable exported, and currently I think we need the export in both the hpp and the cpp.

HunterBelanger · 2024-08-16T15:06:08Z

I just pushed some small changes that now allow compilation with MSVC again.

paulromano

@HunterBelanger This mostly looks good to me at this point. It would be nice to have a minimal CI action that builds the C++ code on the windows-latest runner though. Can you take a stab at that? Otherwise, I know we are at high risk of breaking the build on Windows as new features get added as none of our devs are using Windows.

HunterBelanger · 2024-08-16T17:06:12Z

I completely agree that having CI for Windows will be necessary, as it will quickly be difficult for me to keep up if I am the only Windows compiler at the moment. I have no problems with attacking the CI, but before I can do that, we probably want to consider how we handle the HDF5 dependency.

On Windows, Python now requires that you give it explicit permission to load a DLL that is not in the python path. Because of this, if we link the OpenMC executable to any shared library, the Python API will not work, as it won't be able to load the libopenmc.dll. For a basic build on Windows, the only library we need to link against is libhdf5. Currently, I have installed HDF5 locally on my system using the HDF5 group installer. When I build OpenMC on Windows, I pass -DHDF5_USE_STATIC_LIBRARIES=ON to make sure that HDF5 is statically linked. This avoids needing to tell Python that I will allow it to load libhdf5.dll with os.add_dll_directory(dll_dir). Since HDF5 is the only dependency that we don't automatically fetch for users, I envision this being problematic down the road. I am wondering if we don't want to automatically download, build, and link to the HDF5 static library when building on Windows, to avoid these potential problems ? Also, I am otherwise now sure how to get the HDF5 dependency into the Windows CI container.

paulromano · 2024-08-16T17:41:26Z

You can see a list of available tools for the Windows runner here. I know very little about Windows packaging but it looks like vcpkg might be the way to go? From a quick search, they have packages for HDF5, pugixml, libpng, fmt, and xtensor.

gridley · 2024-08-16T17:46:10Z

That'd be one way to do it. If you all look at how paraview distributes binaries, one approach is to roll all the DLLs into one standalone folder and distribute it with OpenMC. Obviously it takes up extra space, and would not be suitable for development, but being able to just download and run a binary without docker on windows would be pretty epic I think.

https://www.paraview.org/download/

HunterBelanger · 2024-08-16T17:53:47Z

@gridley, once we get this in, I think it would even be reasonable to make OpenMC pip installable at that point, greatly simplifying everything for most users.

@paulromano, I forget things like vcpkg exist. I will try to setup a Windows CI using that to grab the HDF5 dependence. On Windows, do we only want to do a basic build, or do we want a limited case matrix ? In theory, MPI should also work on Windows, though I haven't yet tried it, and am not sure anyone would even use it in practice.

paulromano · 2024-08-16T18:03:14Z

@HunterBelanger I'm OK with just a basic build at this point. We can build up the CI one step at a time.

…to feature/windows

HunterBelanger · 2024-09-19T13:09:17Z

Sorry it has been a while, but I have been having a very hard time getting the CI Windows runners to build OpenMC. I have created a set of powershell scripts to help automate the process like we have for bash, and from what I can tell, they appear to work correctly on my PC. For some reason, however, it is not able to find the vcpkg install of HDF5 on the runners, leading the CMake configuration to fail. I tried a few different methods of sshing into a runner to debug the problem, but after trying several different actions, I don' t think this is possible on the Windows runners. If someone has more experience with the Windows CI runners or any ideas, I would greatly appreciate them !

pshriwise · 2024-09-19T20:00:26Z

Sorry it has been a while, but I have been having a very hard time getting the CI Windows runners to build OpenMC. I have created a set of powershell scripts to help automate the process like we have for bash, and from what I can tell, they appear to work correctly on my PC. For some reason, however, it is not able to find the vcpkg install of HDF5 on the runners, leading the CMake configuration to fail. I tried a few different methods of sshing into a runner to debug the problem, but after trying several different actions, I don' t think this is possible on the Windows runners. If someone has more experience with the Windows CI runners or any ideas, I would greatly appreciate them !

Hi! I did a few runs of CI on my fork and was able to log onto the Windows machine using tmate. I also needed to make some additions to the .ps1 script to ensure that the installation script indicates a failure in the workflow.

You can find my changes here in case they're useful:
https://github.com/pshriwise/openmc/tree/feature/windows

HunterBelanger · 2024-09-20T13:19:45Z

Thanks for this, @pshriwise ! I will try using your branch to see if I can figure out why vcpkg is having a hard time on the runners.

HunterBelanger added 14 commits January 17, 2024 13:12

Replace M_PI with PI constant for compiling on Windows with Intel.

47f0b97

Fix reading in file paths on Windows for xs data.

6f6ebae

Makes cmake compatible with Windows/Intel build.

9eff746

Updates for MSVC/Visual Studio compiler.

d711dde

Make sure atomic has seq_cst with Intel compiler on Windows

c2fc162

Adds macros to export global variable symbols on Windows

722c6e6

Updates Python API to find the dynamic libopenmc.dll on Windows

ae0b297

Adds logic to generat dll on Windows and moves to more recent xtensor…

7c832e2

… version

Fix python parallel bug on Windows in depletion.

45e7e19

Update file_utils tests for Windows

a6eee38

Export some more global variables in the DLL

a1bcd55

Merge branch 'develop' into feature/windows

8731d7f

Fixes formating

5f5bec8

Add directory to openmc dll on Windows systems with python >= 3.8.0

2c82b3a

gridley reviewed Mar 30, 2024

View reviewed changes

openmc/deplete/pool.py Outdated Show resolved Hide resolved

Update openmc/deplete/pool.py

ee58f87

Co-authored-by: Gavin Ridley <gavin.keith.ridley@gmail.com>

HunterBelanger mentioned this pull request Apr 5, 2024

Update xtl and xtensor submodules #2941

Merged

5 tasks

HunterBelanger added 4 commits June 5, 2024 11:51

Merge branch 'develop' into feature/windows

6359969

Use submodules for xtensor again now that they have been updated.

8b58264

Fixes clang-format test

578faba

Merge branch 'develop' into feature/windows

544369e

HunterBelanger mentioned this pull request Aug 13, 2024

Remove resonance reconstruction and Cython dependency #3111

Merged

2 tasks

paulromano added 4 commits August 16, 2024 07:28

Merge branch 'develop' into pr/HunterBelanger/2919

06888e8

Rename OPENMC_WIN_COMPILE to OPENMC_DLL_EXPORTS

ccdb7ee

Remove mention of Python 3.8+

3db9028

Use std::filesystem to get rid of sep_char

d6c7e0a

paulromano added 2 commits August 16, 2024 08:54

Use _WIN32, not _MSC_VER

c609f24

Rename DllExport to OPENMC_API

28c42ef

paulromano requested changes Aug 16, 2024

View reviewed changes

HunterBelanger commented Aug 16, 2024

View reviewed changes

HunterBelanger added 2 commits August 16, 2024 11:03

Change and to && for MSVC compilation

071c946

Fixes to OMP reductions for compilation with MSVC

fde82e2

Fix C++ style

f6e1a04

paulromano reviewed Aug 16, 2024

View reviewed changes

HunterBelanger added 10 commits August 18, 2024 17:11

First attempt at adding Windows to CI

38e11f2

Fix xs download script on Windows

3e1bce2

More annoying Windows fixes

61b7164

Changes USERPROFILE to GITHUB_WORKSPACE to try and make Windows CI work

666e8af

Add caching of xs for windows CI

ac97924

Tries to fix some Windows CI problems

b1480ee

Merge branch 'develop' into feature/windows

e436d8a

Maybe cmake will find HDF5 this time...

7a99ace

Merge branch 'feature/windows' of github.com:HunterBelanger/openmc in…

03d40fd

…to feature/windows

Debug action to explor windows runner

37cf13e

paulromano linked an issue Nov 13, 2024 that may be closed by this pull request

error: use of undeclared identifier 'M_PI' #3175

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Windows Support #2919

Initial Windows Support #2919

HunterBelanger commented Mar 19, 2024 •

edited by paulromano

Loading

gridley commented Mar 30, 2024

paulromano commented Apr 4, 2024

HunterBelanger commented Apr 4, 2024

paulromano commented Apr 5, 2024

paulromano left a comment

paulromano Aug 16, 2024

HunterBelanger Aug 16, 2024

HunterBelanger Aug 27, 2024

paulromano Aug 16, 2024

HunterBelanger Aug 16, 2024

paulromano Aug 16, 2024

HunterBelanger Aug 16, 2024

HunterBelanger left a comment

HunterBelanger Aug 16, 2024

HunterBelanger Aug 16, 2024

HunterBelanger commented Aug 16, 2024

paulromano left a comment

HunterBelanger commented Aug 16, 2024 •

edited

Loading

paulromano commented Aug 16, 2024

gridley commented Aug 16, 2024

HunterBelanger commented Aug 16, 2024

paulromano commented Aug 16, 2024

HunterBelanger commented Sep 19, 2024

pshriwise commented Sep 19, 2024

HunterBelanger commented Sep 20, 2024

Initial Windows Support #2919

Are you sure you want to change the base?

Initial Windows Support #2919

Conversation

HunterBelanger commented Mar 19, 2024 • edited by paulromano Loading

Description

Checklist

gridley commented Mar 30, 2024

paulromano commented Apr 4, 2024

HunterBelanger commented Apr 4, 2024

paulromano commented Apr 5, 2024

paulromano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HunterBelanger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HunterBelanger commented Aug 16, 2024

paulromano left a comment

Choose a reason for hiding this comment

HunterBelanger commented Aug 16, 2024 • edited Loading

paulromano commented Aug 16, 2024

gridley commented Aug 16, 2024

HunterBelanger commented Aug 16, 2024

paulromano commented Aug 16, 2024

HunterBelanger commented Sep 19, 2024

pshriwise commented Sep 19, 2024

HunterBelanger commented Sep 20, 2024

HunterBelanger commented Mar 19, 2024 •

edited by paulromano

Loading

HunterBelanger commented Aug 16, 2024 •

edited

Loading