Workflow document reader fails when a <streq> contains a <cyclestr>

[jimfrimel]

Feature Request ... This was discussed with Chris and Sam .. and it was recommended to add the following request for consideration:

Allow Cycledefs in streq/strneq tags.

Also, being able to allow cyclestr in cycledef tags would be very useful. <cycledef group="12hourly">0 12 * * <cyclestr>@Y</cyclestr> * </cycledef>

In short ... a request had come in from Gus Alaka from HRD for his HWRF workflow where he was trying to check for a forecast hour dependency using cyclestr tag. The following does not work, it generates an error.

<dependency>
  <streq><left><cyclestr>@H</cyclestr></left><right>12</right></streq>
</dependency>

[christopherwharrop]

Hi Jim,

As Sam pointed out in your ticket, the <streq> tag is not an appropriate way to accomplish that goal because it will result in stranded tasks for cycles that don't match that dependency. Gus should use multiple <cycledef> tags with ids and assign tasks to the appropriate <cycledef> IDs.

[christinaholt]

Hey guys! I've run into tough problems like this and made a few mods to Rocoto. Wasn't sure if anyone would ever need them, so never pushed them all back.

I added the option to have the cycledef name in an environment variable so that the script can handle what work needs to be done based on the cycledef we are currently in. An example:

<envar>
     <name>MY_CYCLEDEF</name>
     <value><cyclestr>@g</cyclestr></value>
</envar>

Where @g is filled in with 12hourly at that cycle. It would be filled in with, for example 6hourly if you had that cycldef defined for offset hours.

This isn't a straightforward fix to the request your making, but IIR putting it in a dependency seemed to be a lot more complicated because of when the cyclestr gets filled in for dependencies. I could be mis-remembering that requirement.

I also added another dependency that could potentially be helpful for this type of problem. It allows you to determine whether a particular task exists in a current cycle so that you can make a decision on running downstream tasks. The description for the taskvalid dependency is in PR #13.

I should mention that this is helpful when you want to run a slightly different configuration of the same task at different cycledefs, like maybe a 12 hour forecast at 06 and 18 UTC, and 5 day at 00 and 12 UTC.

[samtrahan]

The failure is not in the dependency handling for <streq> or <strneq>. It is a bug in the name_stringdep. In other words, the workflow document cannot even be read in. Whether users should be using <cyclestr> inside <streq> or not, we do need to make sure the document can be read in. I've given Jim a fix to try out on Jet, and if it works, I'll add a branch shortly.

[christopherwharrop]

Either the bug in name_stringdep, which calls CycleString.to_s without a cycle argument, needs to be fixed such that it does pass a cycle, or the use of compoundTimeString in <streq> and <strneq> needs to be explicitly forbidden and replaced with a simple string.

It make sense to me to support the use of compoundTimeString in those tags despite the fact that there is some potential for inappropriate use.

[samtrahan]

The fix for this is presently in feature/principle-of-least-surprise, but will be moved to its own branch shortly.

[samtrahan]

The fix is now in the feature/streq-parsing-issue branch. I will contact Jim in the NOAA helpdesk and ask him to try it.

[christopherwharrop]

Please explain how that branch addresses this issue. I'm not seeing how your branch fixes it. It doesn't add support for <cyclestr> tags in <streq> and <strneq> tags. Instead, to me, it appears to ignore the <cyclestr> tags, silently not doing what the user asked.

[samtrahan]

Rocoto already supports <cyclestr> tags in <streq>, and the dependency logic already handles them correctly. All that was broken was the code that generates information for rocotocheck. The only bug was in a function generates some text for messages. The text is generated while reading the workflow document, so the cycle information is not available.

You can verify yourself that <cyclestr> works in a <streq> by running Jim's test case. I asked him to try, so that we'll have independent verification.

[christopherwharrop]

Rocoto already supports <cyclestr> tags in <streq>, and the dependency logic already handles them correctly. All that was broken was the code that generates information for rocotocheck. The only bug was in a function generates some text for messages.

That is not true. In the current development branch the schema allows the <cyclestr> tags to be inside <streq> and <strneq> but the code crashes when the XML document parsed and the workflow constructed because the original code does not pass a cycle to the to_s call when evaluating the left/right operands of those tags.

My reading of your feature/streq-parsing-issue branch is that you now simply copy treat <cyclestr> tags inside <streq> and <strneq> as raw text and don't evaluate them. Which is not how it need to be fixed. Either do it correctly and evaluate them, or insist that the data type inside those tags is a String and reject compoundTimeStrings at the validation step.

[samtrahan]

Chris,

The dependency handler in WorkflowMgr::StringDependency does correctly handle the <streq> with <cyclestr>. It always did handle that correctly. The failing code, the code you are looking at, just generates information for rocotocheck messages. That function does not have a cycle when generating the information, so it raised an exception. That code is executed when reading the workflow document, so no Rocoto commands started when there was a <cyclestr> inside a <left> or <right>.

[christopherwharrop]

I see what you mean now. The problem is, the broken code is used when those dependencies are instantiated for rocotorun. I see what you've done now.

[samtrahan]

Chris,

I'm considering this issue fixed. Once you're ready for me to put it in "develop" please let me know.

[christopherwharrop]

Please submit a pull request that contains changes which only pertain to this issue. If you get it in early this week, I will review it quickly. Otherwise, it will have to wait until two weeks from today as I will be unavailable all next week.

[christopherwharrop]

Close. This was fixed with #23