Validation finds unexpected elements

[dsleiter]

Motivation

The validator now returns warnings on any extra fields that it finds.

• Ref Validation find unexpected elements #288 - this is a new PR but taking many changes from this previous PR
• Ref Validator should throw warning if there are extra fields NeurodataWithoutBorders/pynwb#942
• Ref Validation find unexpected elements NeurodataWithoutBorders/pynwb#1178
• Adds docval usage to the spec matching utility classes as discussed here: Ensure all specs are validated #526 (comment)

How to test the behavior?

Simple example:

from hdmf.build import GroupBuilder
from hdmf.spec import GroupSpec, SpecCatalog, SpecNamespace
from hdmf.validate import ValidatorMap

spec_catalog = SpecCatalog()
group_spec = GroupSpec('A test group spec with no attributes', data_type_def='Foo')
spec_catalog.register_spec(group_spec, 'test.yaml')
namespace = SpecNamespace('a test namespace', 'test_core', [{'source': 'test.yaml'}],
                          version='0.1.0', catalog=spec_catalog)
vmap = ValidatorMap(namespace)
extra_attribute = {'x': 42}
builder = GroupBuilder('foo', attributes={'data_type': 'Foo', 'extra_field': 'bar'})
result = vmap.validate(builder)
print(result)

Review and run all new unit tests for full coverage.

Checklist

• Did you update CHANGELOG.md with your changes?
• Have you checked our Contributing document?
• Have you ensured the PR clearly describes the problem and the solution?
• Is your contribution compliant with our coding style? This can be checked running flake8 from the source directory.
• Have you checked to ensure that there aren't other open Pull Requests for the same change?
• Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

[ajtritt]

Code looks great, thanks for taking this on. A few change requests. There's only one real issue (See SpecMatch object). The rest are trivial docstring request.

[dsleiter]

@ajtritt thanks for the review! Good suggestions.

In working through this, I began wondering if it could be beneficial to introduce an AttributeBuilder class. I don't know if this would help or hurt across other parts of hdmf, but in the validator here I think it would help with two things:

1. it would provide a separation between attributes coming from the user api layer (containers) and attributes coming from the data translation layer (builders) so that there wouldn't be a need to define "reserved" attributes as they could be separated
2. attribute names and values would be paired together and would provide functionality for things like builder location and accessing an attribute's parent

If you think this could potentially be beneficial, I can create an issue to discuss in more detail

[dsleiter]

@ajtritt @rly I merged in changes from dev, but unfortunately now we have failing tests.

The cause seems to be due to a combination of three things:

1. the new EnumData which is part of the hdmf-experimental namespace Integrate experimental namespace #545
2. the existing validator can't handle multiple namespaces (were you working on something related to this recently @rly?)
3. because EnumData is not part of the hdmf-common namespace, the validator doesn't know that it inherits from VectorData and thinks that it is an extra field when validating a DynamicTable containing an EnumData - so as of this PR it now returns a warning

I think the ideal solution is to make the ValidatorMap be aware of multiple namespaces, but I think that is probably better as a separate issue, do you both agree?

Until that issue is complete, how would you feel about adjusting H5RoundTripMixin.validate to only fail on validation errors and pass / warn on validation warnings? Or do you have any other suggestions?

[rly]

Nice job hunting down the source of the issue.

Until that issue is complete, how would you feel about adjusting H5RoundTripMixin.validate to only fail on validation errors and pass / warn on validation warnings? Or do you have any other suggestions?

Yes, I think that is a great idea.

[rly]

the existing validator can't handle multiple namespaces (were you working on something related to this recently @rly?)

I was, but it is not yet ready. If you want, you can take a stab at it, but I won't be able to get to it until at least a week post-hackathon.

[dsleiter]

I updated the H5RoundTripMixin to only fail on validation errors and warn on validation warnings

Until that issue is complete, how would you feel about adjusting H5RoundTripMixin.validate to only fail on validation errors and pass / warn on validation warnings?

I'm not entirely certain the warning inside the unit test is valuable and might add noise, so let me know if you'd prefer without the warning and I'll update.

[dsleiter]

I see that many pynwb tests failed due to the new validation warning. Most of them look like they'll be fixed by adding neurodata_type to the reserved attributes, but there might be other extra field warnings beyond that.

As a longer-term strategy (once we fix validation of multiple namespaces), do we want tests to fail on validator warnings or only on errors?

[t-b]

I see that many pynwb tests failed due to the new validation warning. Most of them look like they'll be fixed by adding neurodata_type to the reserved attributes, but there might be other extra field warnings beyond that.

Do you have some examples about pynwb validation warnings due to this PR here?

[dsleiter]

@t-b the errors I was referring to are from the ci/circleci: pynwb-dev-pythons38 job here: https://app.circleci.com/pipelines/github/hdmf-dev/hdmf/2866/workflows/bb203ca4-2953-4094-8247-c80247e99006/jobs/33759

There were many errors happening because the validator was not recognizing neurodata_type as the type_key for pynwb, but thanks for your question, as in describing the cause I determined how to fix the issue. It is now resolved via this commit.

There are a few remaining pynwb tests failing, but most don't appear to have anything to do with this PR. There are 4 failing tests related to extra field warnings, all of them from the integration tests in hdf5.test_misc.TestUnitsIO, but I think these are actually valid warnings:

  ERROR in test test_get_obs_intervals (hdf5.test_misc.TestUnitsIO):
  Traceback (most recent call last):
    File "/home/circleci/project/pynwb/tests/integration/hdf5/test_misc.py", line 63, in test_get_obs_intervals
      ut = self.roundtripContainer()
    File "/home/circleci/project/pynwb/src/pynwb/testing/testh5io.py", line 101, in roundtripContainer
      self.validate()
    File "/home/circleci/project/pynwb/src/pynwb/testing/testh5io.py", line 172, in validate
      raise Exception(err)
  Exception: VectorData (acquisition/UnitsTest/spike_times.resolution): encountered extra field

The schema for Units has a dataset named spike_times which is of type VectorData. spike_types has an additional attribute defined: resolution.
https://github.com/NeurodataWithoutBorders/nwb-schema/blob/e13c24ca24d6d17fa4ce8f3e6f40bd8894432f83/core/nwb.misc.yaml#L177-L200
However, when the validator runs, it compares the spike_times builder against the spec for VectorData, which does not include resolution, and therefore raises the warning.

Should specs that only define neurodata_type_inc be allowed to add fields to a data type?

If not, then I think either spike_times needs to define a new type via neurodata_type_def, or resolution needs to be moved somewhere else.

Do you agree with my evaluation?

[t-b]

@t-b the errors I was referring to are from the ci/circleci: pynwb-dev-pythons38 job here: https://app.circleci.com/pipelines/github/hdmf-dev/hdmf/2866/workflows/bb203ca4-2953-4094-8247-c80247e99006/jobs/33759

There were many errors happening because the validator was not recognizing neurodata_type as the type_key for pynwb, but thanks for your question, as in describing the cause I determined how to fix the issue. It is now resolved via this commit.

Okay, question answered ;)

The schema for Units has a dataset named spike_times which is of type VectorData. spike_types has an additional attribute defined: resolution.
https://github.com/NeurodataWithoutBorders/nwb-schema/blob/e13c24ca24d6d17fa4ce8f3e6f40bd8894432f83/core/nwb.misc.yaml#L177-L200
However, when the validator runs, it compares the spike_times builder against the spec for VectorData, which does not include resolution, and therefore raises the warning.

Should specs that only define neurodata_type_inc be allowed to add fields to a data type?

If not, then I think either spike_times needs to define a new type via neurodata_type_def, or resolution needs to be moved somewhere else.

Do you agree with my evaluation?

Yes I agree. Specifically I would say to

Should specs that only define neurodata_type_inc be allowed to add fields to a data type?

No. This is also my understanding when rereading https://schema-language.readthedocs.io/en/latest/specification_language_description.html#group-specification-keys.

I don't know what the fallout from changing something deep like that is though. If we fix that I think we want to complain already when parsing the spec YAML files.

@rly Any thoughts?

[dsleiter]

No. This is also my understanding when rereading https://schema-language.readthedocs.io/en/latest/specification_language_description.html#group-specification-keys.

Ok, then assuming the spec for Units does violate the schema language, I would propose to:

1. create a bug issue to correct the Units spec (and associated container and object mapper)
2. update the failing pynwb tests to only fail for validation errors and raise a warning on validation warnings as part of Validation find unexpected elements NeurodataWithoutBorders/pynwb#1178

That way we can be unblocked from including the new warnings in hdmf and pynwb and discuss how to fix the spec separately.

If we fix that I think we want to complain already when parsing the spec YAML files.

This seems like a great idea to me!

[t-b]

@dsleiter Sounds good!

The failing pynwb tests are validating old NWB files without cached spec. So yes making the error currently encountered into a warning sounds like a good approach.

EDIT:
I also just tested this branch against files NWBv2 files created by MIES, https://github.com/AllenInstitute/MIES, and validation fails as expected due to additional fields. Nice!

[ajtritt]

Looks good to me, nice work!

[rly]

@dsleiter I'm reviewing this and so far so good. For this PR and the future, could you please enable GitHub Actions in your fork of HDMF if it is not already enabled? I know we've had a lot of back and forth regarding how to trigger CI on forks with varying success. I think doing this would allow the codecov CI run, but I am not 100% sure. I might still need to update the workflow or do something like what is mentioned here https://securitylab.github.com/research/github-actions-preventing-pwn-requests

[rly]

@dsleiter Oh, never mind. It looks like GitHub Actions does run and uploads to https://app.codecov.io/gh/agencyenterprise/hdmf and probably for security reasons, codecov does not comment in this PR and the checks are not shown here. I might look into this more, but it's not that important.

[rly]

Looks all good to me! Thanks @dsleiter!

[rly]

Should specs that only define neurodata_type_inc be allowed to add fields to a data type?

If not, then I think either spike_times needs to define a new type via neurodata_type_def, or resolution needs to be moved somewhere else.

I'll look into this before we release the next version of nwb-schema.

[dsleiter]

Thanks for the reviews and merge @ajtritt and @rly!

@rly RE: codecov - nice to see that the github actions are working and the report is at least uploaded to our codecov. Looks like the comment on the PR is made by a codecov bot - do you have this github app configured for this repo? https://github.com/apps/codecov I will see if we can get it installed on agencyenterprise/hdmf, in which case maybe it'll post from our codecov to any of my PRs here.

[t-b]

@dsleiter Yes, nice work. Especially thanks for picking it up where I left it.