Construct cds prior to UDF_LoadKernels

[aprilnovak]

Describe the bug
The UDF_LoadKernels allows you to define kernel variables that can be accessed on the device, with syntax like kernelInfo["defines/p_myVar"] = 3.5. However, because nrs->cds is set up after calling UDF_LoadKernels, you cannot currently define any kernel variables that use member variables of cds.

To Reproduce
Try to declare a kernel variable like this, in UDF_LoadKernels:

kernelInfo["defines/p_offset"] = nrs->cds->fieldOffset[0];

You will get a seg fault because nrs->cds->fieldOffset[0] doesn't exist yet.

Expected behavior
If nrs exists, it is reasonable to expect that cds also exists, since both represent parts of the problem setup and solve. Users may want to declare kernel variables depending on cds, but the current order of setup prevents that.

Desktop (please complete the following information):

$ uname -a
Linux ulam 5.4.0-74-generic #83-Ubuntu SMP Sat May 8 02:35:39 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

[stgeke]

Defining kernel properties in UDF_LoadKernels() based on runtime quantities like nrs->cds->fieldOffset[0]) will break the pre-compilation feature. Is there another usecase where this is needed? If not, I will close this issue for now.

[aprilnovak]

Well technically you can define kernel properties using nrs->fieldOffset, which is a runtime quantity. Why should the behavior be different between the nrs runtime quantities and the cds runtime quantities?

[stgeke]

It's not any different. Whenever you define a runtime variable as a kernel parameter it will break the pre-compilation feature.

[aprilnovak]

This works:

kernelInfo["defines/p_offset"] = nrs->fieldOffset;

I'm using it in a case right now, and pre-compilation doesn't break.

[stgeke]

Pre-compilation (running nekRS with --build-only) will only work as expected if kernels don’t have to be recompiled at runtime. If a kernel parameter depends on a runtime variable a rebuild (at runtime) is required.

[aprilnovak]

I see. But in that case, there's nothing enforcing me from not doing kernelInfo["defines/p_offset"] = nrs->fieldOffset;. That is, I can break pre-compilation as-is. So why not expand that to allow the experienced user to break pre-compilation if they want to define cds things as kernel variables?

Because you can already break pre-compilation, my suggestion only requests we should not limit people who don't use pre-compilation at all by those requirements (and the code design as-is already follows this paradigm).

[MalachiTimothyPhillips]

Hi April,

Breaking pre-compilation is not something that we are really able to support. If anything, we'd want to guard against using, e.g., nrs->fieldOffset in the UDF kernels. I don't think we have a good way of doing that, however. On some systems, such as Summit, pre-compilation is required. In other words, compiling a kernel during a run is a non-starter. A user may unintentionally specify a kernel property that will break pre-compilation, submit a large job, and then subsequently have it fail. We'd want to limit that possibility as much as is reasonably possible.

[aprilnovak]

I understand your point of view. Here's what I would suggest then.

nrs->usrwrk (i.e. bc->wrk) is the only way for the user to get various field data into the BC functions (like a distance to a wall for turbulence cases, coupling data from MOOSE, etc.). By only providing a single, flat array, the user must have some way to know the field offset so that they can access different "slices" as needed. This is a very general need that is not specific to Cardinal. I propose simply having fieldOffset be available in bcData itself. That would be a compromise between this need and the pre-compilation issue.

The alternative for every Cardinal input is to instead have the user print nrs->cds->fieldOffset[0] in their UDF functions, Ctrl-C the Nek run, write the value down, and then do something like kernelInfo["defines/p_offset"] = 12345;. This is error prone and something I'd like to avoid. Interested to hear your thoughts.

[MalachiTimothyPhillips]

There is a fieldOffset available in bcData that you should be able to use (see:

nekRS/src/core/bcData.h

Line 6 in 723cd46

int fieldOffset;

).

We'll have to give this a little thought whenever we implement nrs->fieldOffset != nrs->cds->fieldOffset[0], e.g. for #148, but for now, this should work without any issue.

I'm going to go ahead and mark this issue as "closed". Please feel free to open it again if you have any additional questions. Thanks!

[aprilnovak]

That's perfect, thanks for pointing me to it!