Add Cardinal to FENIX

[cticenhour]

Closes #38

@meltawila As discussed earlier, please build this version of FENIX and run it against your Cardinal inputs from #30 (and any other inputs you are working on, as you like). Please also add any comments you have on build instructions and usage issues to this thread so that they can be addressed as part of the review process. Thanks!

Requesting @simopier for the formal review once this test run is complete and any comments from that test are fully addressed.

[cticenhour]

Note that some changes to the FENIX recipes need to occur before this can be merged. I am working on a recipe PR that I will get reviewed and merged next week.

[cticenhour]

All - also happy to take feedback on the script configuration currently used here. An alternative is doing everything in the Makefile, which should remove at least one step from the process as currently outlined, and require fewer recipe changes.

[meltawila]

Thank you Casey! I was able to follow your instructions and install FENIX with Cardinal enabled. I ran Cardinal test suite as well and got 384 passed, 254 skipped, 0 pending, 6 FAILED. The failed tests are probably out of the scope of this PR (i.e. one of them uses THM which we're not including here). otherwise it looks great to me!

[simopier]

Converting as draft until we get the new recipe merged.

[simopier]

I think it would be nice to get @loganharbour's review as well.

[cticenhour]

The failed tests are probably out of the scope of this PR (i.e. one of them uses THM which we're not including here).

@meltawila Could also be related to our MOOSE submodule being ahead of Cardinal's; that would be my guess with only a few tests failing.

Thanks for checking with your workflow!

[cticenhour]

With the most-recent documentation commit, a small adjustment to TMAP8 is now required. idaholab/TMAP8#153 is pending review and merge.

[cticenhour]

Based on a discussion today with @loganharbour, I think I will adjust this script slightly and the FENIX Makefile in order to facilitate the running of the script as part of the normal make command. This would bring the build process back to something more in-line with a "normal" MOOSE application and ease potential build complications for users.

[cticenhour]

One last adjustment to MOOSE was required, which has now been merged. Once that makes it to the MOOSE submodule, I think we'll be ready to go.

[moosebuild]

All jobs on d887e3b : invalidated by @GiudGiud

post-submodule update

[GiudGiud]

I think the desired submodule update was not done yet

[moosebuild]

All jobs on d887e3b : invalidated by @simopier

submodule update

[moosebuild]

All jobs on d1b0527 : invalidated by @cticenhour

Invalidating due to recipe update (submodules should be fetched properly now)

[moosebuild]

All jobs on d1b0527 : invalidated by @cticenhour

Fix MOAB typo in recipe

[cticenhour]

The TMAP8 portion of this PR has been superceded by #65, so the TMAP8 components will need to be removed. Differing errors are occurring during the Cardinal-inclusive build on Linux CI and on a local container duplicating the CI environment. I will tackle this when back from England.

[moosebuild]

All jobs on 3abe93c : invalidated by @cticenhour

Recipe changes - Cardinal contrib submodules available

[moosebuild]

Job Documentation on 8473f78 wanted to post the following:

View the site here

This comment will be updated on new commits.

[moosebuild]

Job Coverage on 8473f78 wanted to post the following:

Coverage

No coverage report(s) were found for the base commit 8dfa4a76b5. Full coverage report

This comment will be updated on new commits.

[cticenhour]

The remaining failure is related to compiler warnings coming out of Cardinal/OpenMC. I intend to fix these up (or set them up to be ignored in the case of OpenMC), but that will require 1 libMesh PR, 1 Cardinal PR, and then a follow-on PR here to update Cardinal.

So as not to hold this up any further, I am simply turning off -Werror checking for the debug PR/devel recipe in idaholab/civet_recipes#1841. I have made an issue (#68) that this is a problem that should be resolved per our code standards and code cleanliness guidelines.

I will work on fixing this up as soon as I am able, but it might have to wait until the next FY.

[cticenhour]

@loganharbour This was the intent yes. If this ends up being onerous or disruptive from a CI perspective, we can always make it optional. Is this what you are wary of?

@simopier do you have a preference based on Logan's question?

[loganharbour]

No CI concerns. User concerns. In almost all cases where we have coupling, it's optional. If the application has features that do not depend on the coupling application, it should be able to be optional.

Take a moose dev who is trying to make a patch. IMO don't make them compile cardinal unless they need to.

[cticenhour]

Sounds like we're measuring by degrees here. We very often make folks compile multiple MOOSE-based submodules for an application which are "on" at all times. Cardinal's relative weight is the defining difference, and I don't think it's that onerous since we're only adding OpenMC and DAGMC only - nekRS is explicitly left out.

That being said, I don't have a strong preference; I just wanted to make sure the build and setup worked smoothly for users and devs. The Makefile and source changes to make it optional are not hard to do. As PI, I want to give @simopier the final say.

[loganharbour]

I feel pretty strongly about it. This just isn't how we do it it. Not being able to compile it on its own removes semblance of what product fenix is. And in the future when we make dynamic loading the path forward, you'll have to separate them. Potentially allowing things to mix in to the fenix code base that are cardinal dependent.

[cticenhour]

@meltawila good call - the HPC instructions need to be updated a bit. EDIT: Actually, we do include common directions between files. They are updated automatically with these changes.

However, I don't think we need to add the update and rebuild instructions. They should be provided with the latest moose-dev module. Has this not been your experience?

[moosebuild]

Job Precheck on 7b654de wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/fenix/docs/PRs/53/clang_format/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format dfac9c018af808cd6681cc0097d231578182b28b

[simopier]

@loganharbour, do you want to give this a another review before we merge it?

[cticenhour]

@loganharbour I had already updated the follow-on recipe PR to include it if you want to do it there, or I can pull it out separately.

EDIT: We're doing it separately, idaholab/civet_recipes#1874