Warn users when shell lines get too long?

[hamogu]

Compare the following two commands and their outputs using standard CIAO syntax in the shell:

(i386ciao-4.14) MoritzAirRoseGold ~/projects/Carl> dmextract infile="17896/repro/hrcf17896_repro_evt2.fits[bin sky=circle(344.414438123826700000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000d,-29.62299655482281d,1.0976300191888582'')]" outfile=17896_dmextract.fits clobber=yes bkg="17896/repro/hrcf17896_repro_evt2.fits[bin sky=annulus(344.4144381238267d,-29.62299655482281d,2.1952600383777163'',2.5')-ellipse(17107.811321,15662.528302,16.910357,9.884233,51.227175)-ellipse(14646.461538,16086.923077,56.394447,33.985345,13.201676)-ellipse(18307.084746,16428.915254,26.964187,23.036227,86.904536)-ellipse(16290.846154,16443.846154,26.457026,14.774710,20.879897)-ellipse(16782.376471,18115.835294,32.504998,29.757047,116.825358)-ellipse(16111.240506,18338.177215,37.286899,29.066669,156.171734)-ellipse(16303.500000,14767.375000,21.131929,18.430300,44.660765)]"

# dmextract (CIAO 4.14): WARNING: ignoring opt=pha1: extracting radial profile from sky column.

(i386ciao-4.14) MoritzAirRoseGold ~/projects/Carl> dmextract infile="17896/repro/hrcf17896_repro_evt2.fits[bin sky=circle(344.4144381238267000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000d,-29.62299655482281d,1.0976300191888582'')]" outfile=17896_dmextract.fits clobber=yes bkg="17896/repro/hrcf17896_repro_evt2.fits[bin sky=annulus(344.4144381238267d,-29.62299655482281d,2.1952600383777163'',2.5')-ellipse(17107.811321,15662.528302,16.910357,9.884233,51.227175)-ellipse(14646.461538,16086.923077,56.394447,33.985345,13.201676)-ellipse(18307.084746,16428.915254,26.964187,23.036227,86.904536)-ellipse(16290.846154,16443.846154,26.457026,14.774710,20.879897)-ellipse(16782.376471,18115.835294,32.504998,29.757047,116.825358)-ellipse(16111.240506,18338.177215,37.286899,29.066669,156.171734)-ellipse(16303.500000,14767.375000,21.131929,18.430300,44.660765)]"

zsh: abort      dmextract  outfile=17896_dmextract.fits clobber=yes 

The latter is the same instruction, just with more 0 added. If run in e.g. a jupyter notebook as rt.dmextract( ... long string here ...) I just get a "notebook kernel died" which is even less informative than the "zsh: abort" I get on the command line. Since Python allows almost arbitrarily long strings and string manipulation is easy, it's natural to build up e.g. regions on the sky as long strings as I've done for the background region here. In fact, it's happened to me in several cases where I I simply build up a string from a few elements because it's easy and fast and readable and works well for simple cases and then later run the same code on a more complex region and get crashes.

All the handling in Python works fine. I can set `rt.dmextract.bkg=" ... long string ...", but when I then run dmextract, the entire session crashes. I suggest to check the number of characters in the strings before it is passed to the command line, i.e. before https://github.com/cxcsds/ciao-contrib/blob/main/ciao_contrib/runtool.py#L1808

with a helpful error message like "Parameters exceed string length excepted by CIAO tools and your shell." or maybe even "Parameters exceed string length excepted by CIAO tools and your shell. Consider the use of stackfiles and region files instead of long input strings, see https:\ ... ".

Question: Is that a good idea?

Note 1: The example above is a slightly simplified version of what I did in practice. In reality, I'm not just adding 0, but I reach the line length by having more complex source and background regions. I just made this example to show that the problem is really with the length of the string and not the regions I select.

Note 2: This issue is about adding a helpful error message and preventing crashes of the entire session. I am not asking for specific advice in may case, because I discovered that, due to some unrelated bug, the regions I exclude are the wrong regions anyway...

Note 3: I don't know what the max length is and it might depend on the shell, but it's not hard to just try that out on bash and zsh and Mac and Linux and just take the smallest of those numbers. That will cover 99% of all users and still allows reasonably long strings for normal applications. I don't think we need to be more clever than that.

Note 4: While I looked at dmextract, which is wrapped by CIAOToolParFile the same applies probably to the other classes.

[DougBurke]

Sorry. This isn't going to get addressed for the 4.15 release. There are a number of issues, some of which you've alluded to

• what is the exact length for a single parameter value
• what is the exact length for the full command line
• for parameters that support a stack we can switch a long parameter to a stack, which leads to
  • how long can a single line in a stack file be
• even if we can convert parameters to stacks, the command line could still be too long
• I had thought that we'd ignore all this by using the @@foo.par trick to run a parameter, but obviously not. This likely means though that my comments above about the total line length are irrelevant, and what is actually relevant is the requirement for the parameter file (ie how long a string the param file C API and also the Python API can handle) and then what requirements individual tools have for the parameter lengths (since even if the parameter file can support 2048 characters if it's read into a char tooShort[1024]; array)

Options are (once we decide what 'too long' means)

• error out if we find a string is too long
• warn that the string is likely too long but still try and run the command
• we could try and convert to a stack file if the string is too long (and the parameter supports stacks [*])

[*] I thought we tracked whether a parameter supported a stack, but it looks like we don't (it requires parsing the ahelp XML file which is probably why I haven't bothered so far, but it could be done)

[hamogu]

I'm OK with not addressing this in 4.15, but I do think it's worthwhile looking at at some point.

Out of the three options, I'd go with "error out if we find a string is too long". This is a rare condition (otherwise we'd have heard more complaints about it) and sometimes even indicates a user error (like my example). In many cases, it will be straightforward for the user to address, in particular if the error message says helpful things like " In some cases this can be avoided by using a stack file as input instead - see ahelp XXX or `https://useful_thread_here".

That would be way simpler for us to program and maintain and avoids any risk of introducing even more subtle bugs that happen only when long lines are magically converted to stack files, as we'll never be able to get full tests for that.

[DougBurke]

Unfortunately I don't know what "too long" is. Is it 2048, 1024, 1025, Thursday, or something else? Is it per parameter or the overall length, or can you trigger the per-parameter length even if the overall command-string is not too log? Without information on this we can't do anything sensible here, I claim™.