terrapower / armi

An open-source nuclear reactor analysis automation framework that helps design teams increase efficiency and quality
https://terrapower.github.io/armi/
Apache License 2.0
224 stars 87 forks source link

Create a global Operating System variable at import time #1693

Open john-science opened 5 months ago

john-science commented 5 months ago

I believe the only functional solution would be something like:

  1. In context.py, at "import armi" time, ARMI should determine if this system is Window. Linux, or Mac, and sets a global variable: armi.context.PLATFORM.
  2. If it's not Windows or Linux, we fail immediately? This seems good, but is debatable.
  3. Use the variable armi.context.PLATFORM throughout the codebase.

The goal here is to remove a ton of existing (and future) duplicate catches all around ARMI:

if "darwin" in sys.platform:
    # this
elif "darwin" in sys.platform:
    # that
elif "linux" in sys.platform:
    # stuff
else:
    raise OSError("xyz")

Instead we can do:

if PLATFORM == WINDOWS:
    # this
else:
    # that
jakehader commented 5 months ago

An Enum / Flag might be nice for this versus a string!

drewj-usnctech commented 5 months ago

We have a lot of mac users here despite it not being formally supported, it works well

john-science commented 5 months ago

We have a lot of mac users here despite it not being formally supported, it works well

Well, that's a wrench. Hmm

john-science commented 5 months ago

@drewejohnson I just tried to run the ARMI tests on MacOS and 18 of the 2064 tests failed:

https://github.com/terrapower/armi/actions/runs/8898351718/job/24435316299#step:5:2441

Most of the errors were of the form:

No such file or directory: 'refSmallReactor.yaml'

If that's an easy one to solve, maybe I could add MacOS to the official ARMI CI.

john-science commented 4 months ago

We have a lot of mac users here despite it not being formally supported, it works well

@drewejohnson Just FYI, I just opened a PR to support Macs: https://github.com/terrapower/armi/pull/1713