Add test for inserting the keyword INCLUDE from a PyAction in a paral…

[lisajulia]

…lel run with 4 processes

[lisajulia]

jenkins build this please

[akva2]

is there really any parallel aspect to this ? it seems to me that it would be doing exactly the same on all processes which means adding 20+ secs to the test suite for no good reason.

[lisajulia]

is there really any parallel aspect to this ? it seems to me that it would be doing exactly the same on all processes which means adding 20+ secs to the test suite for no good reason.

Yes, you’re right, the test is to make sure that the functionality to add the kw "INCLUDE" in a parallel run is covered by at least one test. I understand the concern about the added time, yet I'd actually prefer to keep this test, if we want to reduce the test suite duration, I’d rather consider removing the sequential test instead.

[bska]

Just a tangential remark here since I haven't been following the development at all. The "regular" action handling (i.e., for the ACTIONX keyword) does not permit INCLUDE in action block. We're of course free to relax that restriction in our own PYACTION keyword but for what it's worth, I do understand why INCLUDE is not permitted in ACTIONX.

[blattms]

FYI the test for INCLUDE in ACTIONX is from 2021 and our notes mark it as supported since at least 2 years...

[bska]

FYI the test for INCLUDE in ACTIONX is from 2021 and our notes mark it as supported since at least 2 years...

Only by accident–we run essentially a full parse of the action block–and only because we've only ever seen very restricted use if any at all.

If we allow INCLUDE there's nothing to stop me from writing

ACTIONX
  A /
  SOME CONDITION /
/
INCLUDE
  'ExtraSteps.inc' /
ENDACTIO

with ExtraSteps.inc being something along the lines of

TSTEP
  100*12.3 /

That's going to really confuse the rest of the run if it expects to use DATES.

[lisajulia]

FYI the test for INCLUDE in ACTIONX is from 2021 and our notes mark it as supported since at least 2 years...

Only by accident–we run essentially a full parse of the action block–and only because we've only ever seen very restricted use if any at all.

If we allow INCLUDE there's nothing to stop me from writing

ACTIONX
  A /
  SOME CONDITION /
/
INCLUDE
  'ExtraSteps.inc' /
ENDACTIO

with ExtraSteps.inc being something along the lines of

TSTEP
  100*12.3 /

That's going to really confuse the rest of the run if it expects to use DATES.

Sorry for the late reply - yes you're right! Should we remove it from the supported keywords? Or at least display a warning that the user must be cautious?

[bska]

FYI the test for INCLUDE in ACTIONX is from 2021 and our notes mark it as supported since at least 2 years...

Only by accident–we run essentially a full parse of the action block–and only because we've only ever seen very restricted use if any at all.

If we allow INCLUDE there's nothing to stop me from writing

ACTIONX
  A /
  SOME CONDITION /
/
INCLUDE
  'ExtraSteps.inc' /
ENDACTIO

with ExtraSteps.inc being something along the lines of

TSTEP
  100*12.3 /

That's going to really confuse the rest of the run if it expects to use DATES.

Sorry for the late reply - yes you're right! Should we remove it from the supported keywords? Or at least display a warning that the user must be cautious?

Do you mean in the manual? If so, then yes I think we should at least add a warning there and a note that this is subject to change in the future.

Due to the way we're currently parsing INCLUDE statements–effectively resolving the link immediately regardless of context and pasting the contents of the include file directly into the to token stream–we don't have a way of detecting INCLUDE inside an action block and therefore no way of issuing a diagnostic message if the situation happens. As an aside, this mechanism is the reason that INCLUDE "works" in this context right now. I agree that it's very convenient and it allows us factor things like segment definitions out of the action block itself, but as I said that's a quirk of the current parser implementation. If that implementation changes, and it might have to if we're going to support nested action blocks, then we may not be able to continue to "support" this feature.

[lisajulia]

FYI the test for INCLUDE in ACTIONX is from 2021 and our notes mark it as supported since at least 2 years...

Only by accident–we run essentially a full parse of the action block–and only because we've only ever seen very restricted use if any at all. If we allow INCLUDE there's nothing to stop me from writing

ACTIONX
  A /
  SOME CONDITION /
/
INCLUDE
  'ExtraSteps.inc' /
ENDACTIO

with ExtraSteps.inc being something along the lines of

TSTEP
  100*12.3 /

That's going to really confuse the rest of the run if it expects to use DATES.

Sorry for the late reply - yes you're right! Should we remove it from the supported keywords? Or at least display a warning that the user must be cautious?

Do you mean in the manual? If so, then yes I think we should at least add a warning there and a note that this is subject to change in the future.

Due to the way we're currently parsing INCLUDE statements–effectively resolving the link immediately regardless of context and pasting the contents of the include file directly into the to token stream–we don't have a way of detecting INCLUDE inside an action block and therefore no way of issuing a diagnostic message if the situation happens. As an aside, this mechanism is the reason that INCLUDE "works" in this context right now. I agree that it's very convenient and it allows us factor things like segment definitions out of the action block itself, but as I said that's a quirk of the current parser implementation. If that implementation changes, and it might have to if we're going to support nested action blocks, then we may not be able to continue to "support" this feature.

Thanks, yes! Here are two PRs adding notes in the reference manual https://github.com/OPM/opm-reference-manual/pull/496 and in the ACTIONX.md in opm-tests https://github.com/OPM/opm-tests/pull/1288. Can you take a look and let me know what I should potentially change?