Added VALID_TOP_LEVEL_KEYS list

[gmabey]

This seems to me like a marginal improvement comparable to the other VALID_*_KEYS lists.

[jacobagilbert]

Can you help me understand when this would be used?

[bhilburn]

Same question as Jacob, I think. Is there a particular use-case or gap you were trying to solve, @gmabey?

[gmabey]

Hi @jacobagilbert thanks for asking.

After reflecting on this PR, I've concluded that it had two purposes.

1. I'm writing a simple Qt-C++ library that mirrors the form and function of the python files already included in this project. As part of that effort, I wanted to iterate over all of the top-level keys in a metadata file, and check to make sure they were all valid. That's when I realized that I had a question (now keys in the top object #218) about admissibility of keys other than global, captures, and annotations. I realize that validate.py does not iterate of all keys at the top level of the metadata, test whether it's in the set of SigMF-top-keys, then make sure that its value is another object (dict) and iterate over all of its keys -- but that's the approach I'm taking in C++. Since I'm trying to mirror all of the class, method, and constant names, I thought it would be a marginal improvement in the-standard-is-self-evident-in-the-code-ness if VALID_ARCHIVE_KEYS also existed in the python.

2. I'm disappointed at the latency associated with getting responses/reviews/feedback from stakeholders on this project, so I think it was kind of a sarcastic, "let's see if they can respond to the simplest PR" stunt.

Now, I know that we all have other lives, and heck even my response to you is later than I wished (I've been sick with the Rona all week), but some things have been kinda excessively slow IMHO, beyond the preemptive answer to "It seems like some issues take a long time to resolve?" like to the point of negligence. Not all parties, mind you, but some.

In order to try to not be a whiner, but a proactive helper, I have tried to alleviate some of the administrative burden of this project by posting comments to issues that I am not a stakeholder on (other than an interested member of the community) and I plan to continue to do so.

Did contributing this PR add to the noise? Maybe, but at least it got me to therapy :-D

That's where I'm at.

[jacobagilbert]

Your frustration is noted and not unreasonable but please keep in mind that this effort's goals of defining and maintaining a specification that is widely used in operational systems is inherently somewhat slow moving, and we naturally allow most PRs to percolate. Your efforts are not unnoticed, but they seem to have been focused outside of the 1.0 cutline which has received most of the development attention recently for good reason. I would also note that the #sigmf room on chat.gnuradio is also a great place to discuss this. We will respond to all PRs and Issues, but there will be lulls also as maintaining SigMF is not any of our day jobs :)

However you choose to contribute, we hope you stay engaged!

---

Back to the PR here though. I still do not understand how this would be beneficial. When doing key validation it should be done explicitly within the appropriate top level object. Defining a dictionary of lists of valid keys does not seem helpful for either validating top level keys (there are three legal keys for .sigmf-meta files and only one for .sigmf-collection files), nor for validating the keys within a top level object. Perhaps I am missing something?

[gmabey]

Your frustration is noted and not unreasonable but please keep in mind that this effort's goals of defining and maintaining a specification that is widely used in operational systems is inherently somewhat slow moving, and we naturally allow most PRs to percolate.

sigh yes I tried to preempt that response with my reference to the preemptive answer to "It seems like some issues take a long time to resolve?" in the README.md and I'm going to hold my complaint that these delays are in excess of "regular" percolation periods.

Your efforts are not unnoticed, but they seem to have been focused outside of the 1.0 cutline which has received most of the development attention recently for good reason. I would also note that the #sigmf room on chat.gnuradio is also a great place to discuss this. We will respond to all PRs and Issues, but there will be lulls also as maintaining SigMF is not any of our day jobs :)

However you choose to contribute, we hope you stay engaged!

Back to the PR here though. I still do not understand how this would be beneficial. When doing key validation it should be done explicitly within the appropriate top level object. Defining a dictionary of lists of valid keys does not seem helpful for either validating top level keys (there are three legal keys for .sigmf-meta files and only one for .sigmf-collection files), nor for validating the keys within a top level object. Perhaps I am missing something?

The observation I make that wasn't apparent in your remarks is that there are core keys that are valid in multiple top sections, so if there is one grand unified loop that validates all types, then it's more resilient to additions (or, heaven forbid, removals) and unified across top-level keys.

My C++ is very much akin to this python:

for top_key, top_value in top_meta_data.items():
    if top_key in VALID_ARCHIVE_KEYS:
        if top_key == GLOBAL_KEY:
            assert(isinstance(top_value, dict))
            for k,v in top_value.items():
                if k in VALID_ARCHIVE_KEYS[top_key]:
                    make_sure_its_a_valid_type_for_this_core_key(k, v)
        else:
            assert(isinstance(top_value, (list, tuple)))
            for list_item in top_value:
                assert(isinstance(list_item, dict))
                for k,v in list_item.items():
                    if k in VALID_ARCHIVE_KEYS[top_key]:
                        make_sure_its_a_valid_type_for_this_core_key(k, v)

which gracefully handles additions to allowed top-level keys (although certainly no additions are anticipated).

But as validate.py is stuck-ish in some sort of fixed-but-not-pushed state (from what I gather from @Teque5 's comments in #186) I certainly don't want to derail progress on that front ... shrug

[jacobagilbert]

it is very unlikely there will be added top level keys (see the discussion on #218), but I believe I understand what you are doing here now. This will probably sit for a little longer as I want to get the code updates from Teque5 and co integrated before we make decisions here as the validator is definitely broken at the moment and I am curious what becomes of that before we make new validation related changes.

Also one minor point of clarification: a SigMF Archive is a specific thing in SigMF. In this case I believe you mean VALID_SCHEMA_KEYS or VALID_RECORDING_KEYS. Here's the decoder ring for SigMF proper nouns:

"Dataset": a .sigmf-data file
"Schema" / "Metadata": a .sigmf-meta file
"Collection": a .sigmf-collection file
"Recording": a .sigmf-data+ .sigmf-meta file, the core object of SigMF.
"Archive": a .sigmf file which is a tar archive consisting of a .sigmf-data+ .sigmf-meta file

[gmabey]

it is very unlikely there will be added top level keys (see the discussion on #218), but I believe I understand what you are doing here now. This will probably sit for a little longer as I want to get the code updates from Teque5 and co integrated before we make decisions here as the validator is definitely broken at the moment and I am curious what becomes of that before we make new validation related changes.

Yay we agree. Now the stage is truly primed for action.

Also one minor point of clarification: a SigMF Archive is a specific thing in SigMF. In this case I believe you mean VALID_SCHEMA_KEYS or VALID_RECORDING_KEYS. Here's the decoder ring for SigMF proper nouns:

"Dataset": a .sigmf-data file "Schema" / "Metadata": a .sigmf-meta file "Collection": a .sigmf-collection file "Recording": a .sigmf-data+ .sigmf-meta file, the core object of SigMF. "Archive": a .sigmf file which is a tar archive consisting of a .sigmf-data+ .sigmf-meta file

or maybe even better "Archive": a .sigmf file which is a tar archive consisting of one or more .sigmf-data+ .sigmf-meta file pairs, as well as an optional .sigmf-collection file.

[jacobagilbert]

Yes, archives can also have multiple Recordings (and collection files), though the tooling is a bit behind on this.

I would still change the name to reflect that this dictionary of valid keys specifically pertains to recording metadata.

[gmabey]

I agree that's more appropriate. Thanks for your feedback.

[Teque5]

But as validate.py is stuck-ish in some sort of fixed-but-not-pushed state (from what I gather from @Teque5 's comments in #186) I certainly don't want to derail progress on that front ... shrug

Realistically I think someone should just dive in and fix validate.py. I don't think my coworker is ever going to get to it.

[gmabey]

Good to know.

…

On Fri, Jan 28, 2022 at 9:38 PM Teque5 ***@***.***> wrote: But as validate.py is stuck-ish in some sort of fixed-but-not-pushed state (from what I gather from @Teque5 <https://github.com/Teque5> 's comments in #186 <#186>) I certainly don't want to derail progress on *that* front ... shrug Realistically I think someone should just dive in and fix validate.py. I don't think my coworker is ever going to get to it. — Reply to this email directly, view it on GitHub <#217 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVTOUBDZNI3U2RLYSLN6HLUYNVN3ANCNFSM5MKVXKTQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[Teque5]

I've been working on it for a few hours now, let's see what I can come up with today.

[jacobagilbert]

Once this is done, this is ready to merge (and I'll make an appropriately analagous update to #261

[jacobagilbert]

Turns out I can do that :)

Thanks.