FieldRegistry and FieldManager Concept

[psakievich]

This is a draft PR of a concept I've been working on. The idea is to create a FieldRegistry with the basic definition of all the fields we use in nalu-wind. This is a singleton type object because it really just holds the information about each field. We then add a FieldManager class that will register fields defined in the FieldRegistery and throw if the field is not listed in the FieldRegistry. There would be 1 FieldManager per Realm since different fields can be on different Realms.

The ultimate goal of this concept is to move all field registration to the algorithms and kernels. My vision is that in the constructor of any given algorithm/kernel all of the fields that the object will use will be registered via FieldManager. Redundant registry is a no-op so that is not a big deal.

The advantages of this concept are:

1. We ensure that only fields that are being used are registered in the code
2. We no longer need to track pointers to fields on the equation systems, realm etc
3. It simplifies the process of adding a field to the code
4. We can use the registry to print helpful error messages when a field typo occurs in the input deck.

The biggest challenge is that we need to ensure that all the objects are created prior to the mesh finalization.
In the current state this PR does not pass all the unit-tests because this is not enforced yet. However, eventually all of this type of thing will go away because it will be taken care of by the algorithms that are actually being tested.

I'm still fleshing this out a bit, but I'm also considering just moving the stk::mesh::MetaData object in Realm into the FieldManager class i.e. make FieldManager the object that holds MetaData and not Realm. This will be a bigger lift but I think it makes sense since it is leading toward specialization.

One other thing I haven't figured out, is I'd like to have the FieldManager::register_field return the field pointer. This will be nice for tracking the pointer when we actually need to use the field, and as an intermediate step while we refactor out the unnecessary pointer tracking. However due to our abstract field types, I haven't figured out a good way to do this without requiring the input of the field's type at the function call. We can do this but it does defeat some of the encapsulation the FieldRegistry is intended to create.

(PS sorry about the white space diffs. I tried to minimize them.)

[tasmith4]

Thanks @psakievich! I'll review in more detail later, but I just wanted to comment that it doesn't actually matter who holds MetaData. The MetaData is held by the BulkData, so the only real reasons for a MetaData member variable anywhere is (a) so you don't have to call bulk.get_meta or whatever the function is, or (b) if you truly only need the MetaData not the BulkData.

In other words, we can already delete the MetaData member variable from Realm anytime we want, and putting it on the FieldManager is just as simple as passing it by reference into the constructor. So that is probably not as big a lift as you were expecting, but also probably not achieving as much encapsulation as you thought either.

[psakievich]

@tasmith4 yeah that is a good point. I guess I could just put the BulkData on the FieldManager instead and then delete the MetaData as you suggested. The bigger lift is just removing all the syntax like realm_.meta_data() gradually. This is something that will have to be done regardless though if we want to reduce the dependencies on Realm

[tasmith4]

It depends. If the FieldManager truly only uses MetaData (which is what I expect), then it doesn't seem to need to hold BulkData? I don't know where the best place to hold the mesh (i.e. BulkData) is, but off the top of my head it's not more obvious to hold it in FieldManager than Realm.

I see your point about syntax removal, and I do agree it makes sense to not have the Realm handing out MetaData, even if it knows about it through BulkData.

[alanw0]

I think overall it's a good idea, and it's going in the right direction. I think a big benefit of reducing dependencies on Realm, is that it would be easier to set up unit-tests for areas of the code that are separate from Realm. One of the challenges of unit-testing in nalu-wind has been that it is complicated to create a Realm.

[psakievich]

@alanw0 After speaking with @overfelt yesterday we came up with an additional idea for this concept. We can automate the field modify and sync calls by borrowing a concept from good ol' Fortran. The idea is when a field is registered we will also pass in a tag

// some header.h
enum class TAG{INPUT, OUTPUT, INPUT_OUTPUT};

// in a kernel constructor
manager.register_field("velocity", TAG::INPUT, this->local_field_container);

The local_field_container will look something like this:

struct local_field_container{
  std::vector<std::any*> input_fields;
  std::vector<std::any*> output_fields;
  std::vector<std::any*> input_output_fields;
};

and the register call will assign the stk field pointers to the appropriate container during the registration process. Then we can modify the execute function to be as follows:

// this can be in the base class for kernels and algorithms
void sync(){
   for(auto& f : input_fields){
      f->sync_to_device();
   }
   for(auto& f : input_output_fields){
     f->sync_to_device();
   } 
}

void execute(){
  sync();
  execute_impl(); // old execute function
  modify(); // modify is similar to sync but just on output and input_output fields
}

This would require moving to c++17, and I'm not sure I got all the syntax exactly correct in this example, but I think this would be a really good way to automate the field sync/modify calls that have been giving us so much trouble lately. Also we have to move to c++17 eventually due to kokkos, so I think we should just do it sooner than later.

I've been testing out c++17 today and it seems to only introduce small diffs. However there are some fixes needed in Trilinos which I'm going to submit a PR for. While we wait for that to merge we can patch Trilinos via spack-manager.

[alanw0]

@psakievich Yes that's an interesting idea. I might need to think through it a little more, but I like it.

[psakievich]

@alanw0 yeah there are many details to think about here. Especially with the kernels since they are copied to device IIRC. But I think this should be doable. I'm going to just move forward with c++17 which will clean up what I've already got in this PR too. I should have a better prototype that we can discuss by the next nalu-wind NGP meeting.

[psakievich]

Here is the c++17 version of this (just to the point of testing registry and manager): https://github.com/psakievich/nalu-wind/pull/17/files

[psakievich]

I have a Cpp17 version of this which is more concise.