NFEvalFunction.evaluateExternal is not yet implemented

[Francesco Casella]

It eventually should.

[Francesco Casella]

This is the cause of the only one model in MSL 3.2.3 that that does not pass the new frontend.

[Per Östlund]

Replying to casella:

This is the cause of the only one model in MSL 3.2.3 that that does not pass the new frontend.

This is no longer the case, and external functions should generally not be evaluated (it's disabled in the OF by default). While we should probably implement support for it in the NF at some point there's no real use case for it right now.

[Francesco Casella]

Thanks @perost for pointing this out, I missed that :)

[Francesco Casella]

The ​commits on Buildings master branch of May 22 indirectly affected ​60 Buildings models, which now fail because of the external function getTimeSpanTMY3

[OMCompiler/Compiler/NFFrontEnd/NFEvalFunction.mo:178:7-180:92:writable]
Error: Internal error NFEvalFunction.evaluateExternal failed on 
Buildings.BoundaryConditions.WeatherData.BaseClasses.getTimeSpanTMY3, 
evaluation of userdefined external functions not yet implemented

I guess we should try to somehow address this issue, since supporting the Buildings library is one of our goals for 2.0.0

[Per Östlund]

Replying to casella:

The ​commits on Buildings master branch of May 22 indirectly affected ​60 Buildings models, which now fail because of the external function getTimeSpanTMY3

[OMCompiler/Compiler/NFFrontEnd/NFEvalFunction.mo:178:7-180:92:writable]
Error: Internal error NFEvalFunction.evaluateExternal failed on 
Buildings.BoundaryConditions.WeatherData.BaseClasses.getTimeSpanTMY3, 
evaluation of userdefined external functions not yet implemented

I guess we should try to somehow address this issue, since supporting the Buildings library is one of our goals for 2.0.0

I "fixed" this in ​PR #210 by marking external functions as non-structural. The issue in this case was in an if-equations such as:

if canRepeatWeatherFile then
  // Behaviour for when the data file is repeatable (contains an exact multiple of one year).
else
  // Behaviour for when the data file is not repeatable.
end if;

canRepeatWeatherFile is determined by reading the start and end time from an external weather data file via getTimeSpanTMY3, and the logic in the compiler is "if-equation with only parameter conditions => evaluate".

It seems to me like evaluating an external function only because it's used in an if-condition is a bad idea. In this case it would for example mean that the start and end time of the data file is hardcoded into the model while the rest of the data is read at simulation time, which could cause unexpected behaviour if the data file is changed after compilation.

So I introduced the concept of non-structural, which is used to indicate that a parameter expression shouldn't be evaluated unless it's strictly necessary. It's still possible to override this with an Evaluate=true annotation though.

[Francesco Casella]

I agree that considering some parts of a data file structural and some others to be runtime changeable in a somewhat arbitrary way is potentially dangerous.

@mwetter, would you like to comment on this?

[Michael Wetter]

If adding Evaluate=true would solve the problem, then this would perfectly be fine.

Would the annotation need to be added in ConvertTime.mo at the declaration of canRepeatWeatherFile, or upstream in ReaderTMY3.mo at the declaration of the final parameter timeSpan whose values is assigned by this C function?

[Per Östlund]

Replying to mwetter:

If adding Evaluate=true would solve the problem, then this would perfectly be fine.

Would the annotation need to be added in ConvertTime.mo at the declaration of canRepeatWeatherFile, or upstream in ReaderTMY3.mo at the declaration of the final parameter timeSpan whose values is assigned by this C function?

No, the issue is already solved as far as OM is concerned, by not trying to evaluate the call since it's unnecessary to do so. Adding Evaluate=true would in fact cause the issue to reappear. I'm not quite sure what @casella wanted you to comment on.

[Francesco Casella]

I'd like @mwetter to comment on what he would prefer to be the semantics of their code. As I understand, there are three variants:

• the external function is never constant-evaluated, which is good from the point of view of the bulk of data in the datafile, because you don't want to have the data embedded in the source code, and you may want to change the data at runtime by just changing the file but not so good because structural simplifications based on the datafile content cannot be applied
• the external function is evaluated in the context of the if-equation (where it has structural impact) but not elsewhere, so you can change the datafile at runtime, but some parts will be kept as they were at compile time
• the external function is always fully evaluated at runtime, resultin in the whole dataset to be handled by the front-end. This would be obtained by adding the Evaluate=true annotation.

OMC can currently support the first variant only, because it cannot evaluate external functions in the new front-end (yet).

The first question is, would you be fine with this variant?

The second question is: what is the intended runtime behaviour on your side?

[Michael Wetter]

@casella:
I am fine with the first variant. The use case of a user changing the data (of the weather file) after the compilation would be rare.

Regarding the intended runtime behavior, the current implementation causes ConvertTime.mo to be a bit more efficient if canRepeatWeatherFile = false. However, in almost all cases, i.e., whenever a user provides an annual weather file which is the case in more than 99% of the simulations, we have canRepeatWeatherFile = true.

[Martin Sjölund]

We should add support for external functions in newInst, provided that the symbol exists in a library that can be dynamically loaded. Then dlopen + libffi could be used.

I don't think we want the old system of compiling C-files and loading those just to be able to call constructors and other external functions during translation time (that is very, very slow after all).

I have looked into this briefly at a few points in time, but never had enough time to implement it. It seemed feasible at the time though.

[Francesco Casella]

Replying to sjoelund.se:

We should add support for external functions in newInst, provided that the symbol exists in a library that can be dynamically loaded. Then dlopen + libffi could be used.

That would be ideal.

I don't think we want the old system of compiling C-files and loading those just to be able to call constructors and other external functions during translation time (that is very, very slow after all).

Indeed, I remember I used that in some project.

[Per Östlund]

Evaluating external functions in shared libraries via libffi is now implemented in aaf9ee15 (and some minor fixes in 31ad3c3). There are some caveats though:

• External functions defined in ModelicaExternalC are broken on Windows. This is a special case since we link ModelicaExternalC into the compiler, so instead of loading a shared library we look the functions up in the omc executable itself. For some reason this isn't working on Windows, we're investigating why. Fixed in cf838fb0 and 997610d.
• Returning records from an external C function isn't supported yet, but record output parameter are. E.g.:

  output SomeRecordType someRecord;
  external "C" someRecord = extFunc(...)
  

  doesn't work but:

  output SomeRecordType someRecord;
  external "C" extFunc(someRecord)
  

  does. This is because the way we represent record types isn't well suited for the libffi interface, and I've been wanting to change the record types for some time anyway since they cause other issues too. I haven't seen any libraries with external functions that return records, so unless there's demand for this I'm postponing it until I've had time to overhaul our record types.
• Arrays of records are not supported yet, same reason as previous point.

[Francesco Casella]

LGTM.