modelica / ModelicaStandardLibrary

Free (standard conforming) library to model mechanical (1D/3D), electrical (analog, digital, machines), magnetic, thermal, fluid, control systems and hierarchical state machines. Also numerical functions and functions for strings, files and streams are included.
https://doc.modelica.org
BSD 3-Clause "New" or "Revised" License
456 stars 166 forks source link

Modelica.Utilities.Files.exist doesn't work if considered as a pure function #1886

Closed modelica-trac-importer closed 7 years ago

modelica-trac-importer commented 7 years ago

Reported by fcasella on 22 Jan 2016 11:02 UTC The Modelica.Utilities.Files.exist is an impure function, because it returns a different result depending on the state of the file system.

However, lacking the impure specifier, OpenModelica considers it as a pure function and, as long as the string input is a constant literal, moves its evaluation to the initialization phase, yielding wrong results in general.

The proper fix to this problem is to mark the function as impure. However, this is not allowed in Modelica 3.2.

Can we agree on a common strategy for all tool vendors for MSL 3.2.2?


Migrated-From: https://trac.modelica.org/Modelica/ticket/1886

modelica-trac-importer commented 7 years ago

Comment by hansolsson on 1 Mar 2016 10:06 UTC Replying to [comment:49 sjoelund.se]:

Replying to [comment:48 hansolsson]:

However, the optimization of assuming that functions are pure doesn't seem like valid for Modelica 3.2 since impure functions may be called in when-clauses, and there is no way of declaring them pure or impure.

Modelica 3.2 says:

Modelica functions are mathematical functions, i.e. calls with the same input argument values always give the same results. A Modelica function is side-effect free with respect to the internal Modelica simulation state. Specifically, the ordering of function calls and the number of calls to a function shall not influence the simulation state."

So for Modelica 3.2, these optimizations are perfectly reasonable to make.

No, since the bulleted list continues with:

Exception: An impure function is either an impure external function or a Modelica function calling an impure function. An impure function (that may return different values at different calls despite having the same input argument values), may be called from within an impure function, from within a when-equation or when-statement, and during initialization.

Unfortunately with comments in-between. Modelica 3.3 adds the possibility to declare if the function is pure/impure - which makes it better.

modelica-trac-importer commented 7 years ago

Comment by sjoelund.se on 1 Mar 2016 10:15 UTC But that exception is useless since there are no impure functions in Modelica 3.2 (no way to declare functions as such). There is no way to declare them as such, which makes them pure.

modelica-trac-importer commented 7 years ago

Comment by hansolsson on 1 Mar 2016 10:54 UTC Replying to [comment:51 sjoelund.se]:

But that exception is useless since there are no impure functions in Modelica 3.2 (no way to declare functions as such). There is no way to declare them as such, which makes them pure.

There is nothing supporting that conclusion.

The 3.2 specification (going back to 2.1) doesn't state that impure function must be declared as impure, just that there are restrictions on the use of impure functions.

If all functions were pure there would be no need for the exception.

What it states is actually useful:

This implies that:

  Real x=foo(x);

foo must be pure since not called in any of the specific contexts.

  function bar
     input Integer x;
     output Integer y;
   algorithm
      y:=x;
      while y>2 then
         y:=if mod(y,2)==1 then 3*y+1 else div(y,2);
      end y;
   end bar;

Is pure since there are no external functions called.

modelica-trac-importer commented 7 years ago

Comment by sjoelund.se on 1 Mar 2016 11:09 UTC Replying to [comment:52 hansolsson]:

* Impure functions may only be called in when-clauses, in impure functions, during initialization.

This implies that:

  Real x=foo(x);

foo must be pure since not called in any of the specific contexts.

Odd case to list. foo(x) is not time-dependent and can be solved during initialization. This means it could possibly be impure. Except it has an algebraic loop, so it cannot be impure. So it is pure and we can solve it at compile-time... That seems like a totally backwards way of specifying something, which is also why we added the concept of impure for 3.3 (in May 2012, before even 3.2.1 was released).

modelica-trac-importer commented 7 years ago

Comment by hansolsson on 1 Mar 2016 11:19 UTC Replying to [comment:53 sjoelund.se]:

Replying to [comment:52 hansolsson]:

* Impure functions may only be called in when-clauses, in impure functions, during initialization.

This implies that: {{{ Real x=foo(x); }}} foo must be pure since not called in any of the specific contexts.

Odd case to list. foo(x) is not time-dependent and can be solved during initialization.

Well, should have written Real y=foo(x); where 'x' is a normal time-varying variable. So, consider that case instead.

We will then have to clarify if "initialization" means initial equations/algorithms, or also parameter-binding - or even non-time dependent equations.

it could possibly be impure. Except it has an algebraic loop, so it cannot be impure. So it is pure and we can solve it at compile-time... That seems like a totally backwards way of specifying something, which is also why we added the concept of impure for 3.3 (in May 2012, before even 3.2.1 was released).

I agree that it is a backwards way and inelegant - and the 3.3 solution seems better - except possibly some issues on whether print should be impure; and the issue above.

However, the main point is that if an external function (or function calling external function) is called in a when-clause using Modelica 3.2 semantics we cannot assume it is pure.

modelica-trac-importer commented 7 years ago

Comment by fcasella on 1 Mar 2016 11:29 UTC I am sorry, but I think there might be a misunderstanding here.

As as I see it, and as I wrote in ticket:1867#comment:84, the problem is that MSL 3.2r2, section 12.3, mentions "impure external functions", but it doesn't say what an impure external function is. More importantly, it doesn't say how a Modelica tool can decide whether an external function is impure or not. Determining whether an external function is impure or not is not something that we can dismiss as a "tool issue" or "quality of implementation issue", as it basically changes the semantics of the model. That's not the same as, say, having a more robust nonlinear solver or a better tearing algorithm or a smarter symbolic manipulation engine to solve equations in closed form.

My goal here was not to start a flame war, but just to make sure that all tool vendors agree on a way to make sure that the behaviour an important piece of MSL 3.2.2, i.e., the Modelica.Utilities.Files functions, is clearly specified, so that all tools have a chance of doing what is expected. I see this is not happening.

The easy way to do so is to explicitly declare the impure functions as impure, but we can't do that, because it's MLS 3.3. I thought that just adding annotation(__ModelicaAssociation_impure) to the functions and using it equivalently to the impure keyword in the function declaration would have been a good pragmatic solution that saves formal issues and gets us to home base. Apparently that was not the case. Maybe we should reconsider this option.

Anyway, I would kindly ask the tool vendors to come to a consensus on to make sure the Modelica.Utilities.Files package in MSL 3.2.2 has a chance of working correctly in all tools, without resorting to undocumented magic. I'm not asking to solve all problems with impure functions, just please make sure this package will work, which is the reason why I opened this ticket. IMHO, we can't release MSL 3.2.2 (and keep it for a while) if this is unresolved. I don't think it should be that hard to find a solution to this problem.

Thanks!

modelica-trac-importer commented 7 years ago

Comment by fcasella on 1 Mar 2016 11:32 UTC We should also try to get the functions from Modelica.Utilities.Streams to work, because the Files package functions have little use without functions to read and write to the files.

modelica-trac-importer commented 7 years ago

Comment by sjoelund.se on 1 Mar 2016 11:35 UTC Replying to [comment:54 hansolsson]:

We will then have to clarify if "initialization" means initial equations/algorithms, or also parameter-binding - or even non-time dependent equations.

We actually don't. That text is not present for 3.3 (which is the semantics OpenModelica knows). In 3.3, the initial system is not allowed to call impure functions. Which I guess from all these tickets needs to be allowed in 3.3r2 or we need to skip MSL 3.3 and go for 3.4 since it seems mostly useless if we cannot use impure functions in non-structural parameter bindings.

it could possibly be impure. Except it has an algebraic loop, so it cannot be impure. So it is pure and we can solve it at compile-time... That seems like a totally backwards way of specifying something, which is also why we added the concept of impure for 3.3 (in May 2012, before even 3.2.1 was released).

I agree that it is a backwards way and inelegant - and the 3.3 solution seems better - except possibly some issues on whether print should be impure; and the issue above.

The problem with print is that it is really two different functions: print (to the tool message window, which is sort of pure) and print (to file, which is not).

However, the main point is that if an external function (or function calling external function) is called in a when-clause using Modelica 3.2 semantics we cannot assume it is pure.

Which means one should not really use impure functions in 3.2. We really should go for MSL 3.3 soon after the 3.2.2 release and clean up the mess.

Again, OM 1.9.4 implements (mostly) 3.3 semantics so it does not record if a function calls another external function. It just records if it calls another impure function. 3.3 semantics is much easier to implement than 3.2...

modelica-trac-importer commented 7 years ago

Comment by hansolsson on 1 Mar 2016 11:53 UTC Replying to [comment:55 fcasella]:

I am sorry, but I think there might be a misunderstanding here.

As as I see it, and as I wrote in ticket:1867#comment:84, the problem is that MSL 3.2r2, section 12.3, mentions "impure external functions", but it doesn't say what an impure external function is. More importantly, it doesn't say how a Modelica tool can decide whether an external function is impure or not. Determining whether an external function is impure or not is not something that we can dismiss as a "tool issue" or "quality of implementation issue", as it basically changes the semantics of the model.

I didn't try to argue that it is a "quality of implementation issue", but that the Modelica 3.2 semantics make the example well-defined and we cannot assume the external functions are pure in that model.

My main point is at the end - we shouldn't delay 3.2.2 due to issues that existed in 3.2.1 and which cannot be resolved in a good way in 3.2.2.

That's not the same as, say, having a more robust nonlinear solver or a better tearing algorithm or a smarter symbolic manipulation engine to solve equations in closed form.

Treating a pure function as impure is actually similar to that - it's a missed optimization. (The reverse is dangerous.)

My goal here was not to start a flame war, but just to make sure that all tool vendors agree on a way to make sure that the behaviour an important piece of MSL 3.2.2, i.e., the Modelica.Utilities.Files functions, is clearly specified, so that all tools have a chance of doing what is expected. I see this is not happening.

But MSL 3.2.1 also contained that function and MSL 3.2.2 is just a patch to that.

What we primarily want to avoid is that MSL 3.2.2. introduces any regression - but if a specific model fails in a specific tool for MSL 3.2.1 and MSL 3.2.2 there is no regression.

Replying to [comment:57 sjoelund.se]:

Replying to [comment:54 hansolsson]:

We will then have to clarify if "initialization" means initial equations/algorithms, or also parameter-binding - or even non-time dependent equations.

We actually don't. That text is not present for 3.3 (which is the semantics OpenModelica knows). In 3.3, the initial system is not allowed to call impure functions. Which I guess from all these tickets needs to be allowed in 3.3r2 or we need to skip MSL 3.3 and go for 3.4 since it seems mostly useless if we cannot use impure functions in non-structural parameter bindings.

I don't know if we actually need impure functions for that. It could be that we found it is no longer needed.

it could possibly be impure. Except it has an algebraic loop, so it cannot be impure. So it is pure and we can solve it at compile-time... That seems like a totally backwards way of specifying something, which is also why we added the concept of impure for 3.3 (in May 2012, before even 3.2.1 was released).

I agree that it is a backwards way and inelegant - and the 3.3 solution seems better - except possibly some issues on whether print should be impure; and the issue above.

The problem with print is that it is really two different functions: print (to the tool message window, which is sort of pure) and print (to file, which is not).

We might as well treat both as impure.

You don't want to print("\n");print("\n"); to be optimized to one new-line stating that the function is "pure".

However, the main point is that if an external function (or function calling external function) is called in a when-clause using Modelica 3.2 semantics we cannot assume it is pure.

Which means one should not really use impure functions in 3.2. We really should go for MSL 3.3 soon after the 3.2.2 release and clean up the mess.

Well, using impure function in 3.2 is a bit of a mess - but we cannot remove the function from MSL 3.2.2 - and we cannot make them pure.

Clearly releasing MSL 3.3 would be good, but we shouldn't delay 3.2.2 release of issues that we cannot resolve in 3.2.2.

modelica-trac-importer commented 7 years ago

Comment by sjoelund.se on 1 Mar 2016 12:01 UTC Replying to [comment:58 hansolsson]:

But MSL 3.2.1 also contained that function and MSL 3.2.2 is just a patch to that.

No, MSL 3.2.2 is a major release in the semver sense. It both breaks backwards compatibility and adds new features, so it is not a patch.

I don't know if we actually need impure functions for that. It could be that we found it is no longer needed.

I had to remove the impure annotation from quite a few functions because they are used to define parameters. I really think it is necessary. Besides, initialization is a discrete event so if you pretend when initial() was meant to be covered, it could be clarified in the 3.3 spec.

We might as well treat both as impure.

You don't want to print("\n");print("\n"); to be optimized to one new-line stating that the function is "pure".

Sure, but you want to be able to call print("\n") from within a regular function. (Yes, it still works, but you need to declare the function pure; conversion scripts needed?)

modelica-trac-importer commented 7 years ago

Comment by dietmarw on 1 Mar 2016 14:05 UTC So rather then trying to "shoehorn" Modelica 3.3 keywords into MSL 3.2.2 and disguising it as tool vendor annotations only to then push for a quick follow up MSL 3.3.0 where all these "sins" (i.e., vendor annotations) should be removed again from the Modelica Standard Library again why not be honest and make this MSL a MSL3.3.0.

I know it is very late. But most of the latest discussions and problems and ticket opening/closing game seems to be related of workarounds that are necessary because we can not use MLS 3.3 features but it seems that they are really needed in order to make this release a proper stable one. We just seem to be appending the release notes with exceptions.

Also I don't think MSL 3.3.0 would automatically mean conversion script. It can purely mean we are (honestly) using MLS 3.3 features (which basically tools have to support anyway in order to run the current MSL 3.2.2 state, only via vendor annotations) but not all.

This would at least be a clean and standard compliant release without vendor annotations that we are trying to keep out of the MSL.

modelica-trac-importer commented 7 years ago

Comment by fcasella on 1 Mar 2016 14:27 UTC I discussed the issue with Martin S. and he pointed out that MLS 3.3r1 introduces additional restrictions that forbid using impure functions in parameter binding equations and in most types of initialization equations (start values, initial equations, initial algorithms) with the exception of when initial(). These restrictions were not present in MLS 3.2r2.

He thinks these restrictions are too strong, and I agree with him. It seems really odd to me that I cannot write something as

parameter Real p = readFromFile("xxx.txt");
constant Real q = readFromFile("yyy.txt");

if readFromFile is marked as impure, as it should, since we cannot assume that the file contents are never changed.

So, it seems that the MLS 3.3 impure concept is still not ready for a clean and standard solution.

Maybe a pragmatic solution to this ticket is just to add annotation(__ModelicaAssociation_impure) to the Utilities and Streams functions and add in the release notes that this just means "do not optimize the function call assuming the function is pure", without any reference to MLS 3.3

IMHO this is much cleaner and better than tool-specific patches or (God forbid) hacks in the compiler that treat functions whose name is Modelica.Utilities.Files.* in a special way.

My 2 cts.

modelica-trac-importer commented 7 years ago

Comment by hansolsson on 1 Mar 2016 14:35 UTC Replying to [comment:59 sjoelund.se]:

Replying to [comment:58 hansolsson]:

But MSL 3.2.1 also contained that function and MSL 3.2.2 is just a patch to that.

No, MSL 3.2.2 is a major release in the semver sense. It both breaks backwards compatibility and adds new features, so it is not a patch.

Let us not start another discussion about versioning concepts.

My point was that these functions existed in MSL 3.2.1 as well - with the same issues. That is true regardless even if 3 other models were changed in non-backwards compatible ways (which was done since those 3 models were bad before).

I don't know if we actually need impure functions for that. It could be that we found it is no longer needed.

I had to remove the impure annotation from quite a few functions because they are used to define parameters. I really think it is necessary. Besides, initialization is a discrete event so if you pretend when initial() was meant to be covered, it could be clarified in the 3.3 spec.

Can you list exactly which functions and where they are used?

However, as far I understand this primarily means that we need to revisit the 3.3 specification - it could be that we need to improve it, it could be that the function should be marked as pure - or it could be that some other change is the best one.

The second conclusion is that switching from 3.2.2 to 3.3.0 wouldn't be a clean simple solution - but require additional design work. As I see it we should focus on what can be resolved in 3.2.2 and resolve that now - and then examine other versions.

While writing (a second time due to server restart) I noticed that Casella wrote something similar.

We might as well treat both as impure.

You don't want to print("\n");print("\n"); to be optimized to one new-line stating that the function is "pure".

Sure, but you want to be able to call print("\n") from within a regular function. (Yes, it still works, but you need to declare the function pure; conversion scripts needed?)

I have not considered how to automatically convert that, and it is yet another issue that needs to be resolved in order to use 3.3 in practice.

The problem is that it depends on where and how the function is used.

The simple solution will probably be to be forgiving for such errors.

modelica-trac-importer commented 7 years ago

Comment by hansolsson on 1 Mar 2016 15:30 UTC Replying to [comment:61 fcasella]:

I discussed the issue with Martin S. and he pointed out that MLS 3.3r1 introduces additional restrictions that forbid using impure functions in parameter binding equations and in most types of initialization equations (start values, initial equations, initial algorithms) with the exception of when initial(). These restrictions were not present in MLS 3.2r2.

He thinks these restrictions are too strong, and I agree with him. It seems really odd to me that I cannot write something as

parameter Real p = readFromFile("xxx.txt");
constant Real q = readFromFile("yyy.txt");

I created a new ticket for this, #1936

Maybe a pragmatic solution to this ticket is just to add annotation(__ModelicaAssociation_impure) to the Utilities and Streams functions and add in the release notes that this just means "do not optimize the function call assuming the function is pure", without any reference to MLS 3.3

I assume you mean: annotation(__ModelicaAssociation_impure=true);

This made me see that we had such a ticket: #234. I don't like having such an annotation - but if necessary I could live with it.

IMHO this is much cleaner and better than tool-specific patches or (God forbid) hacks in the compiler that treat functions whose name is Modelica.Utilities.Files.* in a special way.

We don't need such tool-specific hacks for this in Modelica 3.2 - it's just a matter of disabling pure function optimizations inside when-clauses, algorithms (in functions at least), and possibly and some other places - since impure functions may be called there.

modelica-trac-importer commented 7 years ago

Comment by otter on 2 Mar 2016 03:51 UTC Based on some email communication, it seems that the proposal from Francesco (https://trac.modelica.org/Modelica/ticket/1886#comment:55) could be a compromize that more people can live with.

The vendor specific impure annotations are removed and replaced by the following Modelica Association impure annotation (with capital "I" in Impure):

annotation(__ModelicaAssociation_Impure = true)

This has the following effect on tools:

If there is no strong opposition against this proposal, I will introduce it in the trunk later in the afternoon today (March 2).

I will also schedule a session in the Modelica design meeting next week, to improve the Modelica 3.3 semantics of impure (this could be formally introduced with a new revision of 3.3).

modelica-trac-importer commented 7 years ago

Comment by sjoelund.se on 2 Mar 2016 08:00 UTC Replying to [comment:63 hansolsson]:

We don't need such tool-specific hacks for this in Modelica 3.2 - it's just a matter of disabling pure function optimizations inside when-clauses, algorithms (in functions at least), and possibly and some other places - since impure functions may be called there.

But that is a tool-specific hack. Modelica Spec 3.3 says that the pure/impure language feature is backward compatible with 3.2. Which implies that a tool that implements proper handling of pure/impure according to the 3.3 specification should be able to handle 3.2 models without any change (which includes evaluating any function not marked impure even if they occur in a when-statement). This is false and pure/impure was a backward-incompatible change. Yes, it can be dealt with by implementing support for 3.2 semantics as well (and switching between the two). Should we open a ticket to clarify this in the specification so that the text is moved to the backward-incompatible section for the next release of the spec?

modelica-trac-importer commented 7 years ago

Comment by hansolsson on 2 Mar 2016 09:25 UTC Replying to [comment:65 sjoelund.se]:

Replying to [comment:63 hansolsson]:

We don't need such tool-specific hacks for this in Modelica 3.2 - it's just a matter of disabling pure function optimizations inside when-clauses, algorithms (in functions at least), and possibly and some other places - since impure functions may be called there.

But that is a tool-specific hack. Modelica Spec 3.3 says that the pure/impure language feature is backward compatible with 3.2.

I see, and to be clear: With Modelica 3.2 semantics it is not a hack. With Modelica 3.3 semantics it is a "hack"/compatibility handling.

Which implies that a tool that implements proper handling of pure/impure according to the 3.3 specification should be able to handle 3.2 models without any change (which includes evaluating any function not marked impure even if they occur in a when-statement). This is false and pure/impure was a backward-incompatible change.

Yes, the semantic changes of pure/impure were backward-incompatible.

I believe we were tricked by the simple idea that pure/impure were new keywords and being allowed to add new specifiers for classes has no impact on existing classes.

However, at the same time the semantics were changed so that you have to add them - which is a breaking change.

Yes, it can be dealt with by implementing support for 3.2 semantics as well (and switching between the two). Should we open a ticket to clarify this in the specification so that the text is moved to the backward-incompatible section for the next release of the spec?

Yes.

modelica-trac-importer commented 7 years ago

Comment by otter on 2 Mar 2016 10:23 UTC Fixed in 1d650577a72039140dd10f3ee2f3364eb9395056 by replacing the vendor specific annotation by the annotation __ModelicaAssociation_impure=true. This also means that there is no longer an impure annotation for

Utilities.Examples.readRealParameter
Utilities.Streams.readRealMatrix

since the impure annotation of OpenModelica was removed from it previously. This is correct (and uncritical), because these two functions give the same result back, if called multiple times (provided the file on the file system did not change in between).

The above change was tested to work in Dymola 2017 Beta 1.

In the meantime, also Jesper (Modelon) send an email, that this change is fine for JModelica.org.

modelica-trac-importer commented 7 years ago

Changelog modified by otter on 2 Mar 2016 10:23 UTC Marked impure functions with the vendor specific annotation __ModelicaAssociation_Impure=true

modelica-trac-importer commented 7 years ago

Comment by beutlich on 3 Mar 2016 07:39 UTC 0361d8e42b550a55fffad323e95f6d621940c3fc updated _annotation(__ModelicaAssociationImpure = true) for the readTableData functions.

modelica-trac-importer commented 7 years ago

Modified by beutlich on 3 Mar 2016 12:57 UTC

modelica-trac-importer commented 7 years ago

Changelog modified by beutlich on 3 Mar 2016 12:57 UTC Marked impure functions with the vendor specific annotation __ModelicaAssociation_Impure=true

modelica-trac-importer commented 7 years ago

Comment by sjoelund.se on 13 Sep 2016 08:16 UTC Something like this is not allowed when using impure, but this pattern is used in MSL functions:

impure function myPrint
  input String s;
  external "C" ModelicaMessage(s) annotation(Include="#include \"ModelicaUtilities.h\"");
end myPrint;

model M
  Real r;
algorithm
  myPrint("Do something");
  r := 1;
end M;

For example in:

Nonlinear.Examples.quadratureLobattoXXX

This means for obvious reasons, the Impure annotation is treated differently from the impure keyword in OpenModelica.

modelica-trac-importer commented 7 years ago

Comment by otter on 14 Sep 2016 05:54 UTC Replying to [comment:70 sjoelund.se]:

Something like this is not allowed when using impure, but this pattern is used in MSL functions:

[..]

For example in:

Nonlinear.Examples.quadratureLobattoXXX

This means for obvious reasons, the Impure annotation is treated differently from the impure keyword in OpenModelica.

With my new proposal:

If an impure function has no output arguments, then no optimizations are allowed with exception of sorting and moving the call to another place (however, removing the call or common subexpression elimination is not allowed). Additionally, there are no calling restrictions (can be called where pure functions are called).

With this backwards compatible change the above situation is handled.

modelica-trac-importer commented 7 years ago

Comment by sjoelund.se on 14 Sep 2016 06:14 UTC Replying to [comment:71 otter]:

If an impure function has no output arguments [...]

This is a very ad-hoc restriction. If you marked an otherwise pure function as impure in order to say that it also write to file / uses debug prints and you don't want the translator to optimize away this function... it would not be allowed to call it simply because it returned a value:

impure /* do not remove me or the print statements inside. I want to trace all calls the tool makes to these functions */ function f
  input Real r;
  output Real o;
algorithm
  print("r before f(): " + String(r) + "\n");
  o := 2*r;
  print("r after f(): " + String(o) + "\n");
end f;

model M
  Real r = time;
  Real o = f(r);
end M;

A more reasonable change (also backwards compatible) would be to say that you can call impure functions from any place and no optimization except for sorting (not allowing moving the call before/after any other impure function call as described above), with the exception of appearances in algebraic loops.