Skip to content
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

Make AMReX and Microphysics submodules of Castro #651

Closed
maxpkatz opened this issue Aug 11, 2019 · 1 comment
Closed

Make AMReX and Microphysics submodules of Castro #651

maxpkatz opened this issue Aug 11, 2019 · 1 comment
Assignees
Projects

Comments

@maxpkatz
Copy link
Member

maxpkatz commented Aug 11, 2019

AMReX and Microphysics should be git submodules of Castro for a lower barrier to entry for new users.

I've sketched out what this looks like in mini-Castro already, for AMReX. We would create directories called amrex and Microphysics in the top-level Castro directory. Then inside those we set a specific commit of each of those codes to check out when you download Castro (which would presumably be the most recent monthly release of each of those codes -- this enforcement of version parity is a side benefit of going this route). A new user getting the code for the first time just has to do

git clone --recursive

and a user that already has the Castro code just has to do

git submodule init ---update

to get the submodules downloaded.

This will not break the workflow of those of us who maintain those dependencies separately, since in the Makefile we will do

AMREX_HOME ?= $(TOP)/amrex

and if you already have AMREX_HOME in your environment it will pick up your other copy of it.

We discussed this a few years ago and decided not to do this, but we should revisit this because my proposed mechanism gives a person the freedom to use the code either way. Also, since it is now very common for large HPC codes to have git submodule dependencies like this now (e.g. most Kokkos- and RAJA-based codes use Kokkos and RAJA as git submodules), I don't think this will be a confusing workflow for users.

A major benefit of this approach for Microphysics is that we would not have to maintain redundant copies of the Microphysics interfaces (and the gamma law EOS) in Castro's Microphysics directory. We could just have Microphysics supply that directly. I recall an argument that was made at the time was that we don't want users to be confused about extra requirements, but aside from my point above that I think most users are familiar with this idea by now, we can also have this be friendly by detecting if either MICROPHYSICS_HOME is set or if the Microphysics directory doesn't yet exist in the top-level directory (because they forgot to do git clone --recursive) and then printing out a helpful error message telling them what to do in that case. I've done this in mini-Castro for AMReX, as follows:

# Include the AMReX make rules. Throw an error
# if we don't have AMReX.
ifeq ("$(wildcard $(AMREX_HOME)/Tools/GNUMake/Make.defs)","")
  $(error AMReX has not been downloaded. Please run "git submodule update --init" from the top level of the code)
endif
include $(AMREX_HOME)/Tools/GNUMake/Make.defs
@adam-m-jcbs
Copy link
Member

adam-m-jcbs commented Oct 23, 2019

tl;dr:

  1. Excellent use-case for submodules
  2. You need to be ready not only for initialization, but also updating of submodules (requires code pullers to manually update or use submodule-aware flags any time an upstream submodule is updated, and gods help them if there are submodule-involved conflicts)
  3. Users should be given some basic info on making submodules usable

It would be hard to come up with a more textbook use-case for git submodules. I support this idea. As someone that has used submodules in my repos, I would add that in addition to preparing users and the build system for initializing submodules, you also need to prepare both for regular updating of submodules.

Every upstream update of a submodule requires all pullers of the parent code to manually update their version of the submodule, unless we can build some robust machinery into git hooks and/or the build system or we make the very dubious assumption that everyone will always do git pull --recurse-submodules on a clean working tree and not have submodule-involved conflicts.

Submodule repos will also often be on detached heads, which to me I thought was an error but is natural for how submodules are intended to be used. Users should be prepared for this in case they're new to submodules. I think there are some tricks to try to keep submodules on master or similar, but I'm not sure they pay off.

Finally, by default git tells you very little useful info about git submodules. Users should know of configuration options that facilitate working with submodules, such as

[status]
    submodulesummary = 1
[diff]
    submodule = log

This article was very useful to me in my frustration with submodules, and could provide some source material for our documentation or any submodule automation we want to implement: https://medium.com/@porteneuve/mastering-git-submodules-34c65e940407

@zingale zingale added this to todo in JOSS paper Jan 1, 2020
@adam-m-jcbs adam-m-jcbs self-assigned this Feb 10, 2020
zingale pushed a commit that referenced this issue Feb 11, 2020
This implements the Microphysics part of #651. We replace the local Castro Microphysics directory with a dependence on the main starkiller-astro Microphysics directory. This means that obtaining Microphysics is now a requirement for using Castro. If the user sets MICROPHYSICS_HOME as before, then their workflow does not change. A new path is that Microphysics is a git submodule of Castro, so if the user does git submodule update --init --recursive then they will obtain the version of Microphysics that we specify in the repo, and if they do not have MICROPHYSICS_HOME set then this submodule will be used. The repo is now updated with git pull --recurse-submodules, so that updates to Microphysics come along when you pull.

A minor associated change is that we've standardized on NETWORK_INPUTS in all of the makefiles, instead of using GENERAL_NET_INPUTS with the hardcoded path.
@zingale zingale closed this as completed Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

3 participants