#2050: cctk_startupxxx is a valid group ---------------------+------------------------------------------------------ Reporter: sbrandt | Owner: sbrandt Type: defect | Status: new Priority: unset | Milestone: Component: Other | Version: development version Keywords: | ---------------------+------------------------------------------------------ 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.
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: new Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ----------------------+-----------------------------------------------------
Comment (by rhaas):
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).
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: new Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ----------------------+-----------------------------------------------------
Comment (by knarf):
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?
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: new Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ----------------------+-----------------------------------------------------
Comment (by knarf):
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.
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: new Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ----------------------+-----------------------------------------------------
Comment (by rhaas):
Was that applied to the release? If so we should close the ticket.
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: new Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ----------------------+-----------------------------------------------------
Comment (by sbrandt):
It was not applied. On the call, everyone said it should wait.
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: new Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ----------------------+-----------------------------------------------------
Comment (by rhaas):
I guess this is something to apply now.
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: new Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ----------------------+-----------------------------------------------------
Comment (by rhaas):
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)?
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: new Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ----------------------+-----------------------------------------------------
Comment (by rhaas):
ping.
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: new Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ----------------------+-----------------------------------------------------
Comment (by sbrandt):
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.
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: review Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ----------------------+----------------------------------------------------- Changes (by sbrandt):
* status: new => review
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: review Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ----------------------+-----------------------------------------------------
Comment (by sbrandt):
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.
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: review Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ----------------------+-----------------------------------------------------
Comment (by rhaas):
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.
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: review Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ----------------------+-----------------------------------------------------
Comment (by sbrandt):
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.
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: review Priority: unset | Milestone: ET_2018_02 Component: Cactus | Version: development version Resolution: | Keywords: ----------------------+----------------------------------------------------- Changes (by sbrandt):
* component: Other => Cactus * milestone: => ET_2018_02
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: review Priority: unset | Milestone: ET_2018_08 Component: Cactus | Version: development version Resolution: | Keywords: ----------------------+----------------------------------------------------- Changes (by hinder):
* milestone: ET_2018_02 => ET_2018_08
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: review Priority: unset | Milestone: ET_2018_08 Component: Cactus | Version: development version Resolution: | Keywords: ----------------------+-----------------------------------------------------
Comment (by rhaas):
Now is the time to apply this.
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: review Priority: unset | Milestone: ET_2018_08 Component: Cactus | Version: development version Resolution: | Keywords: ----------------------+-----------------------------------------------------
Comment (by rhaas):
Pull request with commits is here: https://bitbucket.org/cactuscode/cactus /pull-requests/47/restore-error-on-unknown-timebin-correctly/diff
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: review Priority: unset | Milestone: ET_2018_08 Component: Cactus | Version: development version Resolution: | Keywords: ----------------------+-----------------------------------------------------
Comment (by sbrandt):
Thanks, Roland. Can we move this to review_ok?
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: reviewed_ok Priority: unset | Milestone: ET_2018_08 Component: Cactus | Version: development version Resolution: | Keywords: backport ----------------------+----------------------------------------------------- Changes (by rhaas):
* keywords: => backport * status: review => reviewed_ok
Comment:
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).
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: closed Priority: unset | Milestone: ET_2018_08 Component: Cactus | Version: development version Resolution: fixed | Keywords: backport ----------------------+----------------------------------------------------- Changes (by sbrandt):
* status: reviewed_ok => closed * resolution: => fixed
Comment:
Pull request merged.
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: reopened Priority: unset | Milestone: ET_2018_08 Component: Cactus | Version: development version Resolution: | Keywords: backport ----------------------+----------------------------------------------------- Changes (by rhaas):
* status: closed => reopened * resolution: fixed =>
Comment:
Re-opening until backport is done.
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: review Priority: unset | Milestone: ET_2018_08 Component: Cactus | Version: development version Resolution: | Keywords: backport ----------------------+----------------------------------------------------- Changes (by rhaas):
* status: reopened => review
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: reviewed_ok Priority: unset | Milestone: ET_2018_08 Component: Cactus | Version: development version Resolution: | Keywords: backport ----------------------+----------------------------------------------------- Changes (by rhaas):
* status: review => reviewed_ok
#2050: cctk_startupxxx is a valid group ----------------------+----------------------------------------------------- Reporter: sbrandt | Owner: sbrandt Type: defect | Status: closed Priority: unset | Milestone: ET_2018_08 Component: Cactus | Version: development version Resolution: fixed | Keywords: backport ----------------------+----------------------------------------------------- Changes (by sbrandt):
* status: reviewed_ok => closed * resolution: => fixed
Comment:
Backport is done.
trac@lists.einsteintoolkit.org
-
Einstein Toolkit