Unclear error message for parameter file error

Comments (18)

1. 

   Steven R. Brandt

   • removed comment

   I have a fix. Let's see if this agrees with what you expect. First, here's what I used for a par file.

   # ML_BSSN_FH::block_size_i = 4
   # ML_BSSN_FH::block_size_j = 4
   # ML_BSSN_FH::block_size_k = 4
   
   ML_BSSN_FH::ML_log_confac_bound = "none"
   ML_BSSN_FH_Helper::ML_BSSN_FH_CalculateRHSInMoLPostStep
   

   Here is the error I now produce:

   WARNING level 0 from host sbrand3-5.lsu.edu process 0
     while executing schedule bin (none), routine (no thorn)::(no routine)
     in thorn cactus, file x.par:1:
     -> ERROR IN PARAMETER FILE:Parse Error
   Expected one of the following characters: [\[=]
   
   # ML_BSSN_FH::block_size_k = 4
   
   ML_BSSN_FH::ML_log_confac_bound = "none"
   ML_BSSN_FH_Helper::ML_BSSN_FH_CalculateRHSInMoLPostStep
                                                           ^
   

   This shows the correct line and position in the line, and the set of characters that it expected. Is [[=] too cryptic?

   • 2016-05-18T14:47:48+00:00
2. 

   Roland Haas

   • removed comment

   For all the "expected one of the following character" message from piraha: I would not include the surrounding pair of "[]" since a literal reading of the text would imply that "[" and "]" are allowed characters at this position. Ie the user should not need to know that regular expressions are at all involved in piraha (since this is an implementation details best hidden from the user).

   • 2016-05-20T09:56:57+00:00
3. 

   Frank Löffler

   • removed comment

   Replying to [comment:2 rhaas]:

   For all the "expected one of the following character" message from piraha: I would not include the surrounding pair of "[]" since a literal reading of the text would imply that "[" and "]" are allowed characters at this position. Ie the user should not need to know that regular expressions are at all involved in piraha (since this is an implementation details best hidden from the user).

   I agree in general, but the problem with that might be that these expressions aren't always as simple as a single [] construct, i.e., it is not always just a list of allowed (or not allowed) characters only.

   • 2016-05-20T13:55:58+00:00
4. 

   Frank Löffler

   • removed comment

   Or in other words: I suspect Piraha "knows" at that moment only that some input didn't match a specific regular expression. It doesn't "know" that that particular regular expression might have been a quite simple one. We could add a check for that just for the sake of error messages, but would it be worth it? One also would need to remove the escape for "[", and possibly parse other stuff inside the regular expressions, e.g., a leading !^ (which, interestingly, I had to look up how to escape in this ticket).

   • 2016-05-20T14:03:04+00:00
5. 

   Frank Löffler

   • changed status to open
   • removed comment

   I propose to apply this change (as it improves things considerably), and discuss an even better, more general solution, during a call/meeting.

   • 2016-05-23T09:02:26+00:00
6. 

   Steven R. Brandt

   • removed comment

   Patch updated with clearer error messages. Now you see this:

   WARNING level 0 from host sbrand3-5.lsu.edu process 0
     while executing schedule bin (none), routine (no thorn)::(no routine)
     in thorn cactus, file x.par:1:
     -> ERROR IN PARAMETER FILE:Parse Error
   Expected one of the following characters: '[', '='
   # ML_BSSN_FH::block_size_k = 4
   
   ML_BSSN_FH::ML_log_confac_bound = "none"
   ML_BSSN_FH_Helper::ML_BSSN_FH_CalculateRHSInMoLPostStep
                                                           ^
   
   application called MPI_Abort(MPI_COMM_WORLD, 1) - process 0
   

   and if you add the = sign, you get this

   WARNING level 0 from host sbrand3-5.lsu.edu process 0
     while executing schedule bin (none), routine (no thorn)::(no routine)
     in thorn cactus, file x.par:1:
     -> ERROR IN PARAMETER FILE:Parse Error
   Expected one of the following characters: '[', '+', '-', '!', '(', '0' to '9',double quote, '.', '/', 'a' to 'z', 'A' to 'Z', '$'
   # ML_BSSN_FH::block_size_k = 4
   
   ML_BSSN_FH::ML_log_confac_bound = "none"
   ML_BSSN_FH_Helper::ML_BSSN_FH_CalculateRHSInMoLPostStep =
                                                             ^
   
   application called MPI_Abort(MPI_COMM_WORLD, 1) - process 0
   

   • 2016-05-24T09:28:19+00:00
7. 

   Roland Haas

   • removed comment

   I like the example output that you provide. I am however loath to apply this very close to the release unless it is very clear it cannot make parsing abort. Unfortunately the patch introduces a rather large amount of what seems whitespace only changes making review quite hard. Would you be able to provide a patch with less whitespace changes so that it is easier to see what the "payload" changes are?

   • 2016-05-27T07:43:36+00:00
8. 

   Steven R. Brandt

   • removed comment

   Whitespaces removed.

   • 2016-05-27T10:51:51+00:00
9. 

   Roland Haas

   • removed comment

   hmm, this will take a while I am afraid. The code is still very hard for me to read since it introduces many new lines of code and changes other lines above and below as well. It also seems to be completely uncommented, using very short variable names to give me no hint as to what they may mean, making this a very hard to maintain code I fear.

   One question I have even before digging any deeper: why does the human readable range only include 'a' to 'y' and not 'a' to 'z' (and similar for digits and uppercase letters)? Should the underscore '_' be included in human readable (this would make human readable the class of characters allowed for C identifiers).

   • 2016-06-14T10:12:32+00:00
10. 

    Steven R. Brandt

    • removed comment

    Possibly it's a suboptimal method name. The idea is that if we have two characters, a and b, and a+1 == b, and isHumanReadableRange(a) is true, then it can display as a range, i.e. with a dash between the characters. Thus, z, Z, 9, and _ should not be included.

    • 2016-06-14T16:01:09+00:00
11. 

    Steven R. Brandt

    • changed status to open
    • assigned issue to
    • removed comment

    • 2016-06-27T10:18:48+00:00
12. 

    Roland Haas

    The patch no longer applies. It has also been 4 years since the ticket was created. @Steven R. Brandt which parts of the patch are still needed? If there are any, could you create a pull request please?

    • 2020-08-26T15:12:29+00:00
13. 

    Roland Haas

    The patch no longer applies. It has also been 4 years since the ticket was created. @Steven R. Brandt , @Erik Schnetter which parts of the patch are still needed? If there are any, could you create a pull request please?

    • 2021-02-04T14:20:14+00:00
14. 

    Steven R. Brandt

    @Roland Haas I think the error messages are currently pretty close to what’s above.

    • 2021-02-04T14:58:06+00:00
15. 

    Roland Haas

    Right now if I run Erik’s example I get:

    WARNING level 0 from host 8992d193.ncsa.illinois.edu process 0
      in thorn cactus, file qc0-mclachlan.par:1:
      -> ERROR IN PARAMETER FILE:Parse Error
    Expected one of the following characters: '[', '='
    
    ML_BSSN_FH_Helper::ML_BSSN_FH_CalculateRHSInMoLPostStep
    
    ADMBase::evolution_method         = "ML_BSSN"
    ^
    

    which has no line number (or if “1” is supposed to be the line number, it;s wrong). The caret seems to point to the next line still. So most of the issues reported in the description seem to still be present to me.

    • 2021-02-04T15:43:39+00:00
16. 

    Roland Haas

    The error message is still not optimal since it eg points to the wrong line (though at least it now shows the incorrect line). Since the parfile attachment seems to have vanished I tried with a simpler reproducer:

    Cactus::cctk_itlast = 10
    
    # this line is wrong
    Cactus::cctk_full_warnings
    
    # this line is ok
    Cactus::cctk_run_title = "lineno test"
    

    which fails with:

    Activating thorn Cactus...Success -> active implementation Cactus
    WARNING level 0 from host ekohaes8 process 0
      in thorn cactus, file lineno.par:7:
      -> ERROR IN PARAMETER FILE:Parse Error
    Expected one of the following characters: '[', '='
    Cactus::cctk_full_warnings
    
    # this line is ok
    Cactus::cctk_run_title = "lineno test"
    ^
    
    WARNING level 0 from host ekohaes8 process 0
      in thorn cactus, file lineno.par:7:
      -> ERROR IN PARAMETER FILE:Parse Error
    Expected one of the following characters: '[', '='
    Cactus::cctk_full_warnings
    
    # this line is ok
    Cactus::cctk_run_title = "lineno test"
    ^
    
    --------------------------------------------------------------------------
    MPI_ABORT was invoked on rank 0 in communicator MPI_COMM_WORLD
    with errorcode 1.
    

    maybe instead of pointing to the first wrong character, point to the last good one (not counting whitespace and comments) would be better in this case? Though it obviously comes with its own issues such as this file:

    Cactus::cctk_itlast = 12
    # now an incorrect line
    ::this_does_not_work = 42
    

    which should point out that : is wrong, since the line above could be parsed correctly. Ie the error message would be state dependent (and will almost certainly violate a nice layered approach to the parser).

    • 2023-03-23T17:45:24+00:00
17. 

    Steven R. Brandt

    The problem is, the grammar does not currently see the par file in terms of lines. It treats whitespace much the same as C does. We could revise it to make whitespace important, but this seems of dubious value at best.

    • 2023-03-24T19:57:52+00:00
18. 

    Erik Schnetter reporter

    I think that a parameter file should be interpreted in terms of lines. Humans think in terms of lines, and ignoring lines is confusing.

    • 2023-03-28T14:47:57+00:00
19. Log in to comment