cctk_startupxxx is a valid group

[Steven R. Brandt]

Currently, the "at" keyword in the schedule.ccl file is supposed to report an error if one is scheduling in a group that is not present at startup. It detects this with a regexp, but the regexp does not include boundaries. As such cctk_startupxxx is considered valid.

In addition, the new parser is failing to report a CST error for this violation regardless of group name.

The attached patch to the schedule parser fixes both defects.

Keyword: backport

[Roland Haas]

Sounds like an important fix to have. Your proposed patch is missing a ")" in the English language hint it seems. Why is there a check for level not being 0 in line 973?

Re-stating Erik's worry about MPI (and this seems like a less trivial change to me): are we sure that this will not break any valid schedule.ccl files (anywhere, not just the one in the ET)?

The alternative is to leave it current code in place which does introduce a new bug (which we can document) but at least the bug is not possibly preventing builds (making it major rather than a possible blocker).

[Frank Löffler]

The fix for the regex should go in (line 46), the sooner the better, into the relaase, and you have review_ok for that from me.

As for the rest of the patch:

Lines 200-204 have wrong indentation.

Why do you explicitly abort in line 196 (also, there is some strange indentation going on)? In this case, you already have set level => 0 in $time_bin_info. Couldn't you cause an abort on line 973 instead (removing the if-condition there)? Not directly aborting, but using the latter abort would also not duplicate the definition of $hint. As is, the if-condition on line 973 also makes setting level==0 a noop, but that is done on line 190, so why populate $time_bin_info in line 191 and 192 if that isn't used later?

Also, isn't $where already guaranteed to match schedule_bin_regexp for AT (line 194, already checked in line 563)? Why is this new check necessary?

[Frank Löffler]

Replying to [comment:1 rhaas]:

are we sure that this will not break any valid schedule.ccl files (anywhere, not just the one in the ET)?

Only considering the regex-fix: This will (only) break existing schedule.ccl files if they happened to schedule something at some non-existing timebin that happened to include a valid timebin-string. On the other hand, these were always buggy, and those scheduled items were never actually called in those bins, so never worked anyway. The worst that could happen is that some thorn scheduled something at a non-existent timebin, this was never executed, and also never noticed. This will now result in an CST error. Or in other words: it will complain about broken ccl files, whereas now it silently ignores them. That's good.

[Roland Haas]

Was that applied to the release? If so we should close the ticket.

[Steven R. Brandt]

It was not applied. On the call, everyone said it should wait.

[Roland Haas]

I guess this is something to apply now.

[Roland Haas]

I am somewhat confused though what this patch does. Does it do multiple things? It no longer seems to contain a change to a regex, which the description refers to. Looking at lines 185ff why does scheduling in a non-existing timebin ("AT") trigger a level 0 error right away while for "IN" it is only recorded as a level 1 error (is that a warning?). Shouldn't "AT" and "IN" be essentially identical (after applying this patch)?

[Roland Haas]

ping.

[Steven R. Brandt]

Getting my head back into this issue... So it does two things. First, this fixes the regexp. Second, "at" is broken. It's supposed to produce an error if the group that follows is not one of the basic scheduling bins. This fix, however, broadens the check, allowing any defined group to follow without error. Note that "in" only produces a warning if the group does not exist.

[Steven R. Brandt]

OK, after checking it further... the check is not broadened by this patch. As I recall, it proved controversial, so I took that feature out.

[Roland Haas]

Can you point to what should be reviewed? The ticket is in review state but you say

the check is not broadened by this patch

you want to include this patch or the one that seems to make "at" and "in" behave the same way with the exception that

1. "at" will fail if it is passed a group or bin that does not exist will "in" will silently not schedule a routine in that case (and for "in" this is the required behaviour)
2. "at" will accept the Cactus bins with or without the leading "CCTK_" while "in" requires it?

If the patch is to be applied then it is (or was) somewhat bad code in that it has two similar checks (for bins and groups) in the check_schedule_database and parse_schedule_statement subroutines rather than keeping similar checks in one place.

Also, my

Shouldn't "AT" and "IN" be essentially identical (after applying this patch)? in comment:7 is to be answered in the negative (unless one is very loose in how one interprets "essentially") since AT and IN must differ in how they handle unknown schedule bins / groups which explains the level 0 vs. level 1 warning issue.

I do not understand why $level is set the way it is in line 972 of patch. Shouldn't a non-existing bin in a "IN" statement always be non-fatal. Similarly I am quite confused what $time_bin_info->{$thorn}->{$name}->{$where} may be. It seems like it contains information about group $where in thorn $name but then why would there be a $level associated with it and why would a level 0 error be triggered if that $level is '''non-zero'''?

It seems to me as if this patch is the result of a couple of revisions of the code that changed what the code wants to do and that it should be cleaned up to do the thing it now wants to do in a cleaner way.

[Steven R. Brandt]

This patch (1) fixes the regexp for the time bins, which was in error because it lacked \b's at the start and end, and (2) restores the fatal error for the use of "AT" (which was accidentally demoted to a warning).

$level was present because I wanted to give the user the option of having a fatal error. In this version of the patch it is always 1.

"Where" refers to the bin being scheduled. I am attaching a more streamlined version of the patch.

[Roland Haas]

Now is the time to apply this. Ideally split into two commits: one to handle the regexp for timebins and one to restore the AT behaviour.

[Roland Haas]

Pull request with commits is here: https://bitbucket.org/cactuscode/cactus/pull-requests/47/restore-error-on-unknown-timebin-correctly/diff

[Steven R. Brandt]

Thanks, Roland. Can we move this to review_ok?

[Roland Haas]

Please apply. Please also backport to the release after a week or so.

Please create a new ticket about the case issue that I mentioned in the comments on bitbucket:

The error message outputs the incorrect timebin name in all uppercase. It should use the exact spelling that appears in schedule.ccl so that one can search for it (plus mixed case is easier to read).

[Steven R. Brandt]

Pull request merged.

[Roland Haas]

Re-opening until backport is done.

[Steven R. Brandt]

Backport is done.