ISIS Control Network PVL Header Block Ends in Null Character; not PVL Specification Compliant

[jlaura]

ISIS version(s) affected: 6.0.0 (though likely all versions)

Description
The PVL header that ISIS writes to a control network is terminated with null characters. As per this issue that is not compliant with the specification. This makes reading and ISIS control networks in other packages non-intuitive.

How to reproduce

Read the network in an editor where you can view the characters.

Possible Solution

Alter the control network to terminate the PVL header in a PVL specification compliant way. As per the linked issue this can be:

1. whitespace (which is defined in the PVL-spec and is currently identical for all of the grammars, and is any of: " ", "\t", "\n", "\r", "\v", "\f")
2. a valid PVL comment
3. a semi-colon (;)
4. or the end of the "octet stream" of bytes.

Additional context

[rbeyer]

Since you are placing additional bytes in the file after the PVL-text, just ending the file (4) probably isn't workable. The preferred thing would be to change END to END; (3) as it is explicit. Although changing END to "END " or END\n (1) would also probably be fine choices.

[jlaura]

Looking at some blobdump output, it looks like the same method (null characters) is being used. I haven't looked in the code base, but that makes sense that the same writer is being used in both cases.

It would be awesome to get compliant with the specification.

[jlaura]

I am seeing this as well if I blob dump an isis footprint. The difference here is that the nulls are prepended to the data. So the file looks like this:

PVL HEADER
NULLS <-------------- why?!?!?
ASCII

[github-actions]

Thank you for your contribution!

Unfortunately, this issue hasn't received much attention lately, so it is labeled as 'stale.'

If no additional action is taken, this issue will be automatically closed in 180 days.

[github-actions]

Thank you for your contribution!

Unfortunately, this issue hasn't received much attention lately, so it is labeled as 'stale.'

If no additional action is taken, this issue will be automatically closed in 180 days.

[rbeyer]

Probably still relevant.

[jlaura]

@AustinSanders We should either slot this in the next support sprint or lose with a will not fix. It has been a year and not gotten any traction.

[acpaquette]

@jlaura Would this be considered a breaking change?

[jlaura]

Reading the API Definition I do not think so. This is a fix to PVL that will not change any keys.

[acpaquette]

Hey @jlaura, @chkim-usgs and I were digging into this as a good first issue. We ended up finding no issues with the ISIS PVL control network header and it is valid for ISISGrammer and the OmniGrammer coming from planetarypy's PVL:

This network file was produced by writing out a fresh control network file in a small test that Kim and I cooked to see the issue. This seems more directly linked to how plio is writing ISIS control network headers here:

https://github.com/USGS-Astrogeology/plio/blob/d741d0795eada432a47b82ec8b8a2ed0e6822755/plio/io/io_controlnetwork.py#L474

By specifying the end delimiter be set to false, the header ends up writing no end character and as @rbeyer points out in the original issue in planetarypy the read actually pukes by reading past the END of the PVL header.

It seems like ISIS by default uses \n for the end delimiter and this should probably be reflected in the ISISGrammer in planetarypy's PVL. Then the end delimiter in that plio line should be set to true.

[jlaura]

Is the ISIS PVL output valid with the PVL specification? I think your screen shot above demonstrates that it is not. Sure PVL has a custom grammar for ISIS, but it is because ISIS is a spec unto itself. This issue is to get the header compliant with the PVL spec.

[acpaquette]

In this case the PVL spec is invalid for how ISIS handles comments in PVL. ISIS handles comments in PVL with a # as the opening character and a \n as the closing character. The OMNIEncoder and ISISEncoder in pvl both handle this case only the PVLEncoder doesn't. The OMNIEncoder is also set to the default encoder for the pvl package.

I am not sure updating ISIS to format new comments to PVL spec and then be able to read both old and new comments is within the scope of this ticket.

[jlaura]

In this case the PVL spec is invalid

In 99.9% of cases, specs are not invalid. They are the specification for a reason.

The reported issue has to due with terminating after END. See Ross' comment above. I am not sure what this has to do with comments.

Alternatively, mark as won't fix and close (I do not support this approach, but I am not working on this issue).

[rbeyer]

The original issue is not about how plio uses the pvl library for writing out PVL (which may be a different valid issue), but about how the ISIS C++ programs write out PVL-text. What character is after the "END" token written by the C++ elements of ISIS? When this issue was originally raised, there was an ASCII NULL character (ASCII 0, Hex 0x0) after the "D" in "END" in the example file Jay provided that was written out by a C++ element of ISIS.

That is the issue, ASCII NULL characters terminating a stream of PVL-text is not part of the PVL spec. The original post above lists the four possible acceptable ways to end a stream of PVL-text, and an ASCII NULL character is not one of them.

At the time of the original post, the pvl library's OmniParser failed with that ASCII NULL-terminated PVL-text, but was since adapted so that it accepts it, however, as Jay points out, the PVL-text is still non-conformant to the PVL spec.

You're right, if there are #-delimited comments, that is also not part of the PVL spec (again the Omni parser handles it). Up to you all how much you want to make the PVL-text output by ISIS C++ programs to be conformant to the specification. Right now it isn't at all.

[rbeyer]

I'll note that one of the many failures of PVL as a specification is that almost no one is strictly conformant to it.

[acpaquette]

@jlaura Sorry, I mean't that the PVL ISIS is producing is invalid according to the PVL spec. From the above examination of ISIS PVL header, ISIS invalidates the spec due to how ISIS handles its comments.

But, again the problem with the NULL terminating character is not coming out of ISIS. The NULL terminator is coming out of the pvl package as it doesn't add an end_delimiter due to the nature of the ISISEncoder. ISIS doesn't care about the NULL terminator and can read it in just fine. It's when the network that came out of plio tries to go back into plio using the pvl package, you encounter the original problem posted in planetarypy.

This is the network file that Kim and I produced from ISIS writing out a control network:
whatever.net.zip

Here is the header read out as binary. There is a \n after the END

In terms of fixing this I am unsure, again the ISIS produced PVL header is invalid to the spec with regards to the comments on the file.

If we want ISIS to adhere to verifying the NULL terminator, that is something we can do but will not solve this issue. Again the problem with the NULL terminator is not something that ISIS is producing.

I feel like we are talking past each other and it might be best to touch base over a teams call and get this sorted.

[chkim-usgs]

See planetarypy/pvl#106 for further discussion.

[rbeyer]

I'm confused, on what basis is this Issue resolved? @jlaura, do you consider this Issue closed?

[jlaura]

I'm not sure why this or the issue on PVL have closed. Has any code been changed as per the discussion here or on pvl#106?

[chkim-usgs]

Sorry for the confusion, the issue will be addressed in plio and the PR will be linked here when ready.

[rbeyer]

@jlaura, pv#106 was closed by @acpaquette pursuant to the discussion there. The summary is that the Python pvl library is doing the "right" thing to the extent of its scope when creating PVL-text from Python data structures (via encode(), dump(), or dumps()). If some user code is using pvl to produce PVL-text and then intends to "add" more data to the end of the PVL-text or file that pvl produces (which is beyond pvl scope), it is that user code's responsibility (e.g. plio) to ensure that when additional data is added to the produced PVL-text, that the PVL-text is still parseable to whatever extent that user-code deems important.

I'll note that the default situations for some PVL dialects terminate the PVL-text statements (including the END statement) with a semi-colon which would allow data of any kind to be immediately placed in the stream following the pvl-produced PVL-text. Some dialects do not default to placing a semi-colon at the end of their PVL-text. If user-code is using one of those dialects, then the user-code must place a semi-colon or whitespace of some kind after the PVL-text before placing other data in the same stream after the PVL-text.

The ISIS dialect does not currently end with whitespace or a semicolon. There was discussion about whether the ISIS dialect encoder should always add a whitespace character after the last "D" in "END" in the pvl-produced PVL-text by default, but it was not known what unknown consequences that might have.

Therefore, it was agreed that pvl is doing the "right thing" within scopes that it can control, and no code changes are needed in pvl at this time. If you disagree, please re-open pvl#106.

My question to you is: is this Issue about PVL-text produced by plio or PVL-text produced by C++ ISIS programs? Seems to me that if this was about plio you would have posted this Issue under the plio repo, but maybe not?

[acpaquette]

Closed via DOI-USGS/plio#193

[jlaura]

This was a plio issue. Thanks @acpaquette for identifying that. I thought that this was an ISIS issue, since this network has been read/written using ISIS, but looking at the entries (Apollo data), I am quite condifent that it originated with plio. So, I believe this is good to go! Thanks @acpaquette and @chkim-usgs for digging into this and identifying where the issue was. Thanks @rbeyer for the iteration on PVL as a specification and how to go about fixing this.