Skip to content

Conversation

@aprilnovak
Copy link
Contributor

Adds MOOSE as a submodule in the contrib directory.

@helen-brooks if you like this idea, I can then add all the other dependencies as submodules as well.

@aprilnovak aprilnovak changed the title Add MOOSE as submodule Add MOOSE as submodule and flatten Dec 14, 2022
@aprilnovak
Copy link
Contributor Author

I decided to also add most of the effort needed to flatten the code hierarchy, i.e. #32.

A few items I could use some help with:

  • Does AuroraApp need to exist? I think the answer to this will be "no" if you could, say, have open_mc-opt -i input.i, where input.i just had regular MOOSE syntax (i.e. nothing specific to this repo). Hopefully this is clear what I mean.
  • Do all those unit tests need to exist? Some seem to be checking intrinsic MOOSE features, like instantiating an App object. The reason I ask is because I'm not sure how to have a single unit test directory with objects registered to two different apps (so about half the unit tests fail right now). The easiest fix would be to just delete those (potentially) unnecessary tests.
  • I didn't touch any objects inside the aurora/openmc directory except the src/, include/, test/, and unit/ directories. There's some scripts and other files still there - @helen-brooks I think you should be the one to move those upwards into a directory structure that makes sense since I'm not well versed in their use case.

@helen-brooks
Copy link
Contributor

I'm not especially keen on the idea of adding MOOSE as a submodule. Not only will this potentially bloat the repository, but how does it scale when we consider the wider ecosystem? This application is designed to be interoperable with many other fusion-related MOOSE applications, and apply the same logic to each I would end up with multiple copies of MOOSE which is extremely undesirable. At best I then I have redundant copies of what is already a very large repository, and at worse the linker gets confused because I might have conflicting versions installed. The same reasoning applies to other co-dependencies. Alternatively AURORA gets treated differently to the other fusion apps - but to me this seems rather strange, I'd rather have e.g. AURORA, Achlys, Apollo, be on equal footing.

I can't see a compelling argument to include MOOSE as submodule. If it is a more streamlined user experience you are looking for, I've been developing installation scripts in #30 that should be highly configurable.

@aprilnovak
Copy link
Contributor Author

  • Submodules don't bloat repositories. All they do is to store the hash of a git commit that you want to point to. See the size of the "added" MOOSE repo:

Screen Shot 2022-12-14 at 7 25 46 AM

  • Submodules are a robust way to ensure that dependency versions are enforced, because a specific hash is intimately tied to the repo. Right now, it's the maintainer's responsibility to keep those versions up to date across all the installation scripts and docker files, when it could instead be versioned with the repo itself.
  • Submodules are the standard MOOSE approach for representing dependencies. The average MOOSE user coming to Aurora would immediately do the following to fetch all dependencies:
git submodule update --init

This one-liner is much simpler than cd repo_location; git clone repo for all of the different dependencies. I'm trying to increase the uniformity of Aurora to look more akin to other MOOSE apps.

  • There's no issue with each MOOSE App (Aurora, Achlys, etc.) having their own submodules. At the end of the day, you'd just pull the submodules in one of the applications (or in some parent application, like in Cardinal we have a MOOSE submodule and just skip pulling the individual MOOSE submodules in Sockeye, SAM, etc.):
git clone aurora
git submodule update --init
export MOOSE_DIR=aurora/moose
make -j8

git clone achlys
# skip pulling the submodules because I indicated I want to use the ones in Aurora above
make -j8

@aprilnovak
Copy link
Contributor Author

But ultimately it's your call of course! :)

One other advantage with submodules is that Aurora users would not have to be responsible for setting MOOSE_DIR/DAGMC_DIR/etc. themselves, because when using the submodule these locations are known. Of course the same argument can be made with the installation scripts, but the one I have developed locally is ~150 lines long (and no other MOOSE Apps I've worked with have a script requirement, so other people may not be expecting to need it).

@makeclean
Copy link

Sounds like a win to me ...

@helen-brooks
Copy link
Contributor

helen-brooks commented Dec 14, 2022

I do understand how submodules work, by bloating I was imagining having many users with many applications each with their own copy of MOOSE and thus many different redundant installations (and all this blowing up a bit). Our sysadmins already don't like the fact we each individually install moose, and would prefer it if we just had single system-wide install).

OK, but if we are arbitrarily deciding to sometimes not use submodules, it will still continue to be necessary to support having the build externally, so the user will still have to set MOOSE_DIR and so on.

New users won't need to write their own build scripts, the new build script I've written is completely configurable with command lines arguments.

I still don't quite get the argument about what is to be gained, surely I will still need curate an understanding of compatible versions, whether that happens to be via a git submodule file or via versioned build scripts.

@aprilnovak
Copy link
Contributor Author

I can remove the MOOSE submodule commit if you like. Just to address your points:

so the user will still have to set MOOSE_DIR and so on

With submodules, you don't need the user to set MOOSE_DIR, but they still have the option for more advanced setups. I think right now, the user must set MOOSE_DIR, so it's an extra step (but please correct my understanding if this is not the case).

I will still need curate an understanding of compatible versions, whether that happens to be via a git submodule file or via versioned build scripts

Yes, totally agree. IMO, the advantage of submodules is that it's very clear which MOOSE hash each App is compatible with. As long as the build scripts did something like

git clone moose
git checkout s7fjdj3         # a specific hash

then the scripts would have the same robustness as the submodule approach. So yes, the same effect could be achieved with the build scripts, but in terms of conformity with the rest of the MOOSE ecosystem, all other Apps use the submodule approach (to my knowledge).

@helen-brooks
Copy link
Contributor

helen-brooks commented Dec 15, 2022

I can remove the MOOSE submodule commit if you like.

I think that might be best, at least until I've merged the new build scripts and updated docker files. If we change the build recipe we'll have to consolidate those anyway to pass CI.

@aprilnovak
Copy link
Contributor Author

Sounds good! I have removed it.

@aprilnovak aprilnovak changed the title Add MOOSE as submodule and flatten Flatten Aurora code hierarchy Dec 15, 2022
Copy link
Contributor

@helen-brooks helen-brooks left a comment

Choose a reason for hiding this comment

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

Thanks for separating off the submodule discussion. I'm generally happy with the idea of flattening the structure and agree it'll make things less confusing. A few small changes on the makefiles. Otherwise, the main thing is to get the tests to pass: assuming everything still builds OK, I expect it's a question of just updating the dockerfiles.

  • docker/aurora-fedora/Dockerfile
  • docker/aurora-ubuntu/Dockerfile
  • README.md (user instructions for build)
  • We'll need to regenerate the html docs as well (though I'd be happy to make this merge seperately).

Comment on lines -29 to -45
CHEMICAL_REACTIONS := no
CONTACT := no
EXTERNAL_PETSC_SOLVER := no
FLUID_PROPERTIES := no
FUNCTIONAL_EXPANSION_TOOLS := no
HEAT_CONDUCTION := yes
LEVEL_SET := no
MISC := yes
NAVIER_STOKES := no
PHASE_FIELD := no
POROUS_FLOW := no
RDG := no
RICHARDS := no
SOLID_MECHANICS := no
STOCHASTIC_TOOLS := no
TENSOR_MECHANICS := yes
XFEM := no
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could leave the extra modules in the makefile? Someone might want to enable them for their own purposes.

Comment on lines +9 to +11
AURORA_DIR := $(abspath $(dir $(word $(words $(MAKEFILE_LIST)),$(MAKEFILE_LIST))))
CONTRIB_DIR := $(AURORA_DIR)/contrib
MOOSE_SUBMODULE ?= $(CONTRIB_DIR)/moose
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're not doing the submodule thing I don't think we need these lines?

Comment on lines -29 to -43
CHEMICAL_REACTIONS := no
CONTACT := no
FLUID_PROPERTIES := no
HEAT_CONDUCTION := yes
MISC := no
NAVIER_STOKES := no
PHASE_FIELD := no
RDG := no
RICHARDS := no
SOLID_MECHANICS := no
STOCHASTIC_TOOLS := no
MISC := yes
TENSOR_MECHANICS := yes
XFEM := no
POROUS_FLOW := no
LEVEL_SET := no
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer to keep these options for now.

@helen-brooks
Copy link
Contributor

helen-brooks commented Dec 16, 2022

Ok a few more comments on these things

  • Does AuroraApp need to exist? I think the answer to this will be "no" if you could, say, have open_mc-opt -i input.i, where input.i just had regular MOOSE syntax (i.e. nothing specific to this repo). Hopefully this is clear what I mean.

Probably not(?) I think it could probably go. But I like the name AuroraApp better than OpenMCApp, so maybe the latter is the one to get rid of?

  • Do all those unit tests need to exist? Some seem to be checking intrinsic MOOSE features, like instantiating an App object. The reason I ask is because I'm not sure how to have a single unit test directory with objects registered to two different apps (so about half the unit tests fail right now). The easiest fix would be to just delete those (potentially) unnecessary tests.

I wrote every test to fulfil a function, so yes they need to stay. No doubt some will now fail after these changes, we'll need to fix them.

  • I didn't touch any objects inside the aurora/openmc directory except the src/, include/, test/, and unit/ directories. There's some scripts and other files still there - @helen-brooks I think you should be the one to move those upwards into a directory structure that makes sense since I'm not well versed in their use case.

I think we have:

  • doc : content should be merged with aurora/doc - a lot of this is autogenerated static code documentation but there's some yaml too.
  • performance: some scripts I wrote ages ago for performance benchmarking. It's a bit rough and ready, and perhaps should be removed entirely from this repo - but I'd want to maybe migrate it somewhere else first, so hold off on deleting it.
  • scripts: was auto-generated by MOOSE's stork script. This can be deleted.
  • .gitignore: : was auto-generated by MOOSE, and only modified once. It is superceded by the gitignore in the top level dir, safe to delete.
  • .clangformat: safe to delete there's one in the directory above, and in any case so far I haven't enforced linting. Although we probably should.
  • perf_test.i : again, pertained to performance testing, let me move this somewhere else.
  • run_tests: auto-generated, safe to delete
  • testroot: auto-generated, safe to delete

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.

3 participants