From 85548948474ae383366740c1ed1c808a42a03766 Mon Sep 17 00:00:00 2001 From: Kay Robbins Date: Wed, 30 Sep 2026 12:02:48 -0500 Subject: [PATCH 1/4] Sidecar strings skip the required-tag check Copilot review of PR #1437. A sidecar string is a fragment of a row, so the sidecar stage no longer runs the required-tag check on it; required tags split between a sidecar string and the HED column validate clean. The unique-tag check still runs there, since a unique tag repeated inside a fragment is repeated in the row and hed-tests has a sidecar-only TAG_NOT_UNIQUE case. run_full_string_checks takes required_tags and unique_tags in place of whole_row; the HED-cell pass passes both False. --- hed/validator/hed_validator.py | 25 +++++++++++++++++------ hed/validator/sidecar_validator.py | 5 ++++- hed/validator/spreadsheet_validator.py | 11 +++++++--- tests/validator/test_hed_validator.py | 22 ++++++++++++++++++++ tests/validator/test_staged_validation.py | 17 +++++++++++++++ 5 files changed, 70 insertions(+), 10 deletions(-) diff --git a/hed/validator/hed_validator.py b/hed/validator/hed_validator.py index 2420bf37..7b69ca60 100644 --- a/hed/validator/hed_validator.py +++ b/hed/validator/hed_validator.py @@ -94,14 +94,16 @@ def run_basic_checks(self, hed_string, allow_placeholders) -> list[dict]: issues += self._def_validator.validate_def_tags(hed_string) return issues - def run_full_string_checks(self, hed_string, whole_row=True) -> list[dict]: + def run_full_string_checks(self, hed_string, required_tags=True, unique_tags=True) -> list[dict]: """Run all full-string validation checks on a HED string. Parameters: hed_string (HedString): The HED string to validate. - whole_row (bool): If False, the string is one cell of a row whose other columns also carry HED, - so the checks that need every tag of the row (required and unique tags) are skipped. The - group-level checks still apply: a group in the cell is a group of the row. + required_tags (bool): Run the required-tag check. Pass False for a fragment of a row (a HED cell, + a sidecar string): the tag it lacks may come from another column, so the check needs the + assembled row. + unique_tags (bool): Run the unique-tag check. A unique tag repeated inside a fragment is repeated + in the row too, so this check is safe on a fragment; pass False to leave it to assembly. Returns: list[dict]: A list of issues found during validation. Each issue is represented as a dictionary. @@ -111,14 +113,25 @@ def run_full_string_checks(self, hed_string, whole_row=True) -> list[dict]: - Each check is a callable function that takes `hed_string` as input and returns a list of issues. - If any check returns issues, the method stops and returns those issues immediately. - If no issues are found, an empty list is returned. + - The group-level checks (tag placement, reserved groups, duplicates, Onset and Offset) always + run: a group in a fragment is a group of the row. """ + + def row_tag_checks(string_obj): + tags = string_obj.get_all_tags() + issues = [] + if required_tags: + issues += self._group_validator.check_for_required_tags(tags) + if unique_tags: + issues += self._group_validator.check_multiple_unique_tags_exist(tags) + return issues + checks = [ + row_tag_checks, self._group_validator.run_tag_level_validators, self._def_validator.validate_onset_offset, ] - if whole_row: - checks.insert(0, self._group_validator.run_all_tags_validators) for check in checks: issues = check(hed_string) # Call each function with `hed_string` diff --git a/hed/validator/sidecar_validator.py b/hed/validator/sidecar_validator.py index 8b37d0cc..c47648a0 100644 --- a/hed/validator/sidecar_validator.py +++ b/hed/validator/sidecar_validator.py @@ -123,8 +123,11 @@ def _validate_sidecar(self, sidecar, extra_def_dicts, error_handler) -> list[dic modified_string = df_util.replace_ref(modified_string, f"{{{ref}}}", ref_dict[ref]) hed_string_obj = HedString(modified_string, hed_schema=self._schema, def_dict=sidecar_def_dict) + # A sidecar string is a fragment of a row: a tag it lacks may come from another + # column, so the required-tag check waits for assembly. A unique tag repeated in + # the fragment is repeated in the row, so that check runs here. error_handler.push_error_context(ErrorContext.HED_STRING, hed_string_obj) - new_issues += hed_validator.run_full_string_checks(hed_string_obj) + new_issues += hed_validator.run_full_string_checks(hed_string_obj, required_tags=False) error_handler.add_context_and_filter(new_issues) issues += new_issues error_handler.pop_error_context() # Hed string diff --git a/hed/validator/spreadsheet_validator.py b/hed/validator/spreadsheet_validator.py index fd7b2fac..1f0e4c9a 100644 --- a/hed/validator/spreadsheet_validator.py +++ b/hed/validator/spreadsheet_validator.py @@ -36,7 +36,8 @@ class SpreadsheetValidator: The stages run in order and each stops the run when it finds an error (never on a warning alone): - 1. **Sidecar**: every template and categorical string of the sidecar (``SidecarValidator``). + 1. **Sidecar**: every template and categorical string of the sidecar (``SidecarValidator``). A sidecar + string is a fragment of a row, so the required-tag check waits for assembly. 2. **Column values**: the column structure (mapper issues, sidecar references to columns the table lacks), then each value column's distinct values against the units or value class of its ``#`` tag, and each categorical column's distinct values against its sidecar keys. @@ -394,7 +395,9 @@ def validate_hed_column(self, column_name, distinct, error_handler, row_offset=0 hed_string = HedString(text, self._schema) string_issues = self._hed_validator.run_basic_checks(hed_string, allow_placeholders=False) if full_string and not check_for_any_errors(string_issues): - string_issues += self._hed_validator.run_full_string_checks(hed_string, whole_row=False) + string_issues += self._hed_validator.run_full_string_checks( + hed_string, required_tags=False, unique_tags=False + ) issues += self._report_distinct(string_issues, hed_string, rows, column_name, error_handler, row_offset) return issues @@ -405,7 +408,9 @@ def _validate_hed_cells(self, column_name, distinct, error_handler, row_offset) issues = [] for text, rows in distinct.items(): hed_string = HedString(text, self._schema) - string_issues = self._hed_validator.run_full_string_checks(hed_string, whole_row=False) + string_issues = self._hed_validator.run_full_string_checks( + hed_string, required_tags=False, unique_tags=False + ) issues += self._report_distinct(string_issues, hed_string, rows, column_name, error_handler, row_offset) return issues diff --git a/tests/validator/test_hed_validator.py b/tests/validator/test_hed_validator.py index 4d6c50f2..2cfc213c 100644 --- a/tests/validator/test_hed_validator.py +++ b/tests/validator/test_hed_validator.py @@ -229,6 +229,28 @@ def test_duplicate_group_in_definition(self): issues = test_string.validate(hed_schema) self.assertEqual(len(issues), 1) + def test_run_full_string_checks_row_tag_flags(self): + """The required-tag and unique-tag checks need the whole row; each can be left out for a fragment, + while the group-level checks always run.""" + schema_path = os.path.join(self.hed_base_dir, "HED8.0.0_added_tests.mediawiki") + validator = HedValidator(schema.load_schema(schema_path)) + + missing_required = HedString("Event, (Onset)", validator._hed_schema) + codes = [issue["code"] for issue in validator.run_full_string_checks(missing_required)] + self.assertEqual(codes, ["REQUIRED_TAG_MISSING", "REQUIRED_TAG_MISSING"]) + codes = [issue["code"] for issue in validator.run_full_string_checks(missing_required, required_tags=False)] + self.assertEqual(codes, ["TEMPORAL_TAG_ERROR"]) + + repeated_unique = HedString( + "Action, Animal-agent, (Event-context, (Red, Blue)), (Event-context, (Green, Yellow))", + validator._hed_schema, + ) + codes = [issue["code"] for issue in validator.run_full_string_checks(repeated_unique)] + self.assertEqual(codes, ["TAG_NOT_UNIQUE"]) + self.assertEqual(validator.run_full_string_checks(repeated_unique, unique_tags=False), []) + codes = [issue["code"] for issue in validator.run_full_string_checks(repeated_unique, required_tags=False)] + self.assertEqual(codes, ["TAG_NOT_UNIQUE"]) + def test_severity_enum_not_string_github_hedit_161(self): """Regression test for critical bug: severity should be ErrorSeverity enum, not string. diff --git a/tests/validator/test_staged_validation.py b/tests/validator/test_staged_validation.py index 491cbeb4..6f71c47b 100644 --- a/tests/validator/test_staged_validation.py +++ b/tests/validator/test_staged_validation.py @@ -293,6 +293,23 @@ def test_required_and_unique_tags_are_checked_only_on_assembled_rows(self): issues = validator.validate(ListColumnSource(columns, base_input=frame)) self.assertEqual([issue["code"] for issue in issues], [ValidationErrors.REQUIRED_TAG_MISSING] * 2) + # Required tags split between a sidecar string and the HED cell: clean through a real assembled input, + # so stage 1 must not hold the sidecar string alone to the required-tag rule either. + split = _sidecar({"condition": {"HED": {"a": "Action"}}}) + frame = _tabular([["onset", "duration", "condition", "HED"], [1.0, 0, "a", "Animal-agent"]], sidecar=split) + self.assertEqual(validator.validate(frame), []) + self.assertEqual( + validator.validate(ListColumnSource({"condition": ["a"], "HED": ["Animal-agent"]}), sidecar=split), [] + ) + + # A unique tag repeated inside one sidecar string is repeated in every row that uses it, so stage 1 + # still reports it (hed-tests TAG_NOT_UNIQUE has a sidecar-only case). + repeated = _sidecar( + {"condition": {"HED": {"a": "(Event-context, (Red, Blue)), (Event-context, (Green, Yellow))"}}} + ) + issues = self.validator.validate(ListColumnSource({"condition": ["a"]}), sidecar=repeated) + self.assertEqual([issue["code"] for issue in issues], [ValidationErrors.TAG_NOT_UNIQUE]) + # The same holds for a unique tag repeated within one cell. columns = { "onset": [1.0], From ef802770797de9b3624e8d4a33403d2ed700a2be Mon Sep 17 00:00:00 2001 From: Kay Robbins Date: Wed, 30 Sep 2026 14:30:28 -0500 Subject: [PATCH 2/4] List schema comparison changes in key order SchemaComparer iterated sets of row keys, extras section names, and attribute names, so the order of the 'Row ... missing in first schema' lines and of an entry's attribute changes changed with the hash seed from run to run. The changelog this feeds (hed-schemas PRERELEASE_CHANGES.md) is a generated, committed file, so every regeneration showed spurious reorderings. The three loops now run in sorted order. Tests pin the order for one-sided rows through _compare_dataframes and through gather_schema_changes. --- hed/schema/schema_comparer.py | 16 +++++++++-- tests/schema/test_schema_compare.py | 43 +++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/hed/schema/schema_comparer.py b/hed/schema/schema_comparer.py index dbaea7a1..75f61648 100644 --- a/hed/schema/schema_comparer.py +++ b/hed/schema/schema_comparer.py @@ -805,7 +805,9 @@ def _check_other_attributes(self, change_dict, section_key, entry1, entry2): unique_inherited_keys = unique_keys all_unique_keys = unique_keys.union(unique_inherited_keys).difference(already_checked_attributes) - for key in all_unique_keys: + # Sorted: a set of strings iterates in an order that changes with the hash seed, and the + # changelog this feeds is a generated, committed file. + for key in sorted(all_unique_keys): is_inherited = key in unique_inherited_keys is_direct = key in unique_keys @@ -857,7 +859,7 @@ def _add_extras_changes(self, change_dict): extras2 = getattr(self.schema2, "extras", {}) or {} all_keys = set(extras1.keys()).union(extras2.keys()) - for key in all_keys: + for key in sorted(all_keys): df1 = extras1.get(key) df2 = extras2.get(key) # An absent section and one present with no rows are the same thing: writers always @@ -1001,6 +1003,11 @@ def _compare_dataframes(df1, df2, key_cols): * "Row missing in second schema" * "Duplicate keys found" * "Column values differ" + + Notes: + - The differences are listed in key order (as text), so two runs on the same frames give the + same list. A set of keys iterates in an order that changes with the hash seed, which put + spurious reorderings into the generated changelog. """ results = [] @@ -1009,7 +1016,10 @@ def _compare_dataframes(df1, df2, key_cols): all_keys = set(df1_indexed.index).union(df2_indexed.index) - for key in all_keys: + def key_text(key): + return [str(part) for part in key] if isinstance(key, tuple) else [str(key)] + + for key in sorted(all_keys, key=key_text): if key not in df1_indexed.index: results.append({"row": key, "cols": None, "message": "Row missing in first schema"}) elif key not in df2_indexed.index: diff --git a/tests/schema/test_schema_compare.py b/tests/schema/test_schema_compare.py index 9a779d94..b4277b6d 100644 --- a/tests/schema/test_schema_compare.py +++ b/tests/schema/test_schema_compare.py @@ -326,6 +326,32 @@ def test_row_added_to_schema2_is_minor(self): self.assertEqual(len(added), 1) self.assertEqual(added[0]["change_type"], "Minor") + def test_one_sided_rows_listed_in_key_order(self): + """Rows present in one schema only are recorded in key order, in either direction.""" + base = pd.DataFrame({"source": ["src1"], "link": ["http://a.org"], "description": ["d"]}) + more = pd.DataFrame( + { + "source": ["src1", "src9", "src3", "src5"], + "link": ["http://a.org", "http://i.org", "http://c.org", "http://e.org"], + "description": ["d", "i", "c", "e"], + } + ) + schema1 = self._make_schema({SOURCES_KEY: base.copy()}) + schema2 = self._make_schema({SOURCES_KEY: more.copy()}) + changes = self._extras_result(schema1, schema2)[SOURCES_KEY] + self.assertEqual( + [c["change"] for c in changes], + [f"Row {key} missing in first schema" for key in ["src3", "src5", "src9"]], + ) + + schema1 = self._make_schema({SOURCES_KEY: more.copy()}) + schema2 = self._make_schema({SOURCES_KEY: base.copy()}) + changes = self._extras_result(schema1, schema2)[SOURCES_KEY] + self.assertEqual( + [c["change"] for c in changes], + [f"Row {key} missing in second schema" for key in ["src3", "src5", "src9"]], + ) + def test_row_removed_from_schema2_is_minor(self): """A row present in schema1 but not schema2 is Minor.""" df1 = pd.DataFrame( @@ -441,6 +467,23 @@ def test_multiple_key_columns(self): self.assertIn("Row missing in first schema", messages) self.assertIn("Column values differ", messages) + def test_differences_listed_in_key_order(self): + """The list is in key order whatever order the frames or the hash seed would give, so a generated + changelog does not reorder between runs.""" + df1 = pd.DataFrame({"key": ["m", "z"], "val": ["x", "old"]}) + df2 = pd.DataFrame({"key": ["z", "c", "a", "b"], "val": ["new", "x", "x", "x"]}) + results = SchemaComparer._compare_dataframes(df1, df2, ["key"]) + self.assertEqual([r["row"] for r in results], ["a", "b", "c", "m", "z"]) + self.assertEqual( + [r["message"] for r in results], + ["Row missing in first schema"] * 3 + ["Row missing in second schema", "Column values differ"], + ) + + df1 = pd.DataFrame({"k1": ["b"], "k2": ["1"], "val": ["x"]}) + df2 = pd.DataFrame({"k1": ["b", "a", "b"], "k2": ["1", "2", "0"], "val": ["x", "y", "z"]}) + results = SchemaComparer._compare_dataframes(df1, df2, ["k1", "k2"]) + self.assertEqual([r["row"] for r in results], [("a", "2"), ("b", "0")]) + class TestDerivableUnitRemoval(unittest.TestCase): """HED 8.5.0 drops listed SI variants (uV) that stay valid through modifiers: Patch, not Major.""" From a8856ac2b34577b2812a9f2b73fb2a165fc59d8d Mon Sep 17 00:00:00 2001 From: Kay Robbins Date: Wed, 30 Sep 2026 15:22:08 -0500 Subject: [PATCH 3/4] Make the row-tag check flags keyword-only Copilot review of PR #1438. required_tags and unique_tags on HedValidator.run_full_string_checks can no longer be passed positionally, so a positional call cannot silently change meaning. The whole_row flag they replace lived on main for one day between PRs #1437 and #1438 and never reached a release, so no compatibility parameter is kept for it. --- hed/validator/hed_validator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hed/validator/hed_validator.py b/hed/validator/hed_validator.py index 7b69ca60..23047bca 100644 --- a/hed/validator/hed_validator.py +++ b/hed/validator/hed_validator.py @@ -94,7 +94,7 @@ def run_basic_checks(self, hed_string, allow_placeholders) -> list[dict]: issues += self._def_validator.validate_def_tags(hed_string) return issues - def run_full_string_checks(self, hed_string, required_tags=True, unique_tags=True) -> list[dict]: + def run_full_string_checks(self, hed_string, *, required_tags=True, unique_tags=True) -> list[dict]: """Run all full-string validation checks on a HED string. Parameters: From f1f2af21a18c47f069829fc91e5683d50d38ac37 Mon Sep 17 00:00:00 2001 From: Kay Robbins Date: Wed, 30 Sep 2026 16:49:01 -0500 Subject: [PATCH 4/4] Report a unique tag repeated inside a HED cell Copilot re-review of PR #1438. The cell pass that runs when no assembly follows, and validate_hed_column(full_string=True), now skip only the required-tag check. A unique tag repeated inside one cell is repeated in every assembled row, so the check cannot false-alarm there; it is reported once per distinct string with a row count. The two paths never run together with assembly, so nothing is reported twice. --- hed/validator/spreadsheet_validator.py | 21 +++++++++------------ tests/validator/test_staged_validation.py | 23 ++++++++++++++--------- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/hed/validator/spreadsheet_validator.py b/hed/validator/spreadsheet_validator.py index 1f0e4c9a..05246e5a 100644 --- a/hed/validator/spreadsheet_validator.py +++ b/hed/validator/spreadsheet_validator.py @@ -43,8 +43,8 @@ class SpreadsheetValidator: tag, and each categorical column's distinct values against its sidecar keys. 3. **HED column**: each distinct string of every HED-tags column, the basic (single-string) checks. For a source that offers no assembly, the group-level checks follow on the same strings, since no - stage 4 will run them on the whole row. The required-tag and unique-tag checks belong to the - assembled row and never run on a cell. + stage 4 will run them on the whole row. The required-tag check belongs to the assembled row and + never runs on a cell; the unique-tag check does, since a repeat inside a cell is a repeat of the row. 4. **Assembly**: the rows assembled from all columns, the group-level checks, and the onset and temporal checks of a timeline file. @@ -230,7 +230,7 @@ def _validate_source(self, source, sidecar, extra_def_dicts, error_handler, row_ # Stage 4: the assembled rows, only for a source that can provide them. The source is asked only # now, so a stage 3 error never costs a table conversion. A source that offers no assembly gets # the group-level checks on each distinct HED cell instead, since no stage 4 will run them on the - # whole row; the required-tag and unique-tag checks stay with the assembled row. + # whole row; the required-tag check stays with the assembled row. base_input = source.as_base_input() if base_input is not None: issues += self.validate_assembled(base_input, error_handler, row_offset=row_offset) @@ -384,8 +384,9 @@ def validate_hed_column(self, column_name, distinct, error_handler, row_offset=0 row_offset (int): Added to the first row of each reported string. full_string (bool): If True, also run the group-level checks on each string that passes the basic checks. For a caller that will never assemble rows; otherwise those checks belong - to the assembly stage, where the row is whole. The required-tag and unique-tag checks - need the assembled row and never run here. + to the assembly stage, where the row is whole. The required-tag check needs the assembled + row and never runs here; the unique-tag check does, since a repeat inside a cell is a repeat + of the row. Returns: list[dict]: The issues found, each with ``row_count``. @@ -395,22 +396,18 @@ def validate_hed_column(self, column_name, distinct, error_handler, row_offset=0 hed_string = HedString(text, self._schema) string_issues = self._hed_validator.run_basic_checks(hed_string, allow_placeholders=False) if full_string and not check_for_any_errors(string_issues): - string_issues += self._hed_validator.run_full_string_checks( - hed_string, required_tags=False, unique_tags=False - ) + string_issues += self._hed_validator.run_full_string_checks(hed_string, required_tags=False) issues += self._report_distinct(string_issues, hed_string, rows, column_name, error_handler, row_offset) return issues def _validate_hed_cells(self, column_name, distinct, error_handler, row_offset) -> list[dict]: """Run the group-level checks on each distinct string of a HED-tags column that has passed stage 3, for a source that offers no assembly. One more parse per distinct string. A cell is part of a row, - so the required-tag and unique-tag checks are left to assembly.""" + so the required-tag check is left to assembly; a unique tag repeated inside the cell is reported.""" issues = [] for text, rows in distinct.items(): hed_string = HedString(text, self._schema) - string_issues = self._hed_validator.run_full_string_checks( - hed_string, required_tags=False, unique_tags=False - ) + string_issues = self._hed_validator.run_full_string_checks(hed_string, required_tags=False) issues += self._report_distinct(string_issues, hed_string, rows, column_name, error_handler, row_offset) return issues diff --git a/tests/validator/test_staged_validation.py b/tests/validator/test_staged_validation.py index 6f71c47b..d90e8224 100644 --- a/tests/validator/test_staged_validation.py +++ b/tests/validator/test_staged_validation.py @@ -268,9 +268,10 @@ def test_base_input_is_not_asked_for_when_an_earlier_stage_stops(self): self.assertEqual([issue["code"] for issue in issues], [ValidationErrors.VALUE_INVALID]) self.assertEqual(source.base_input_calls, 0) - def test_required_and_unique_tags_are_checked_only_on_assembled_rows(self): - """A HED cell is part of a row, so the required-tag and unique-tag checks never run on it alone: the - sidecar may supply the tags. They run in the assembly stage; the group-level checks apply to a cell.""" + def test_required_tags_are_checked_only_on_assembled_rows(self): + """A HED cell is part of a row, so the required-tag check never runs on it alone: the sidecar may supply + the tags. It runs in the assembly stage. The group-level and unique-tag checks apply to a cell, since + what they find inside the cell holds for the row.""" schema_path = os.path.join( os.path.dirname(os.path.realpath(__file__)), "../data/validator_tests/HED8.0.0_added_tests.mediawiki" ) @@ -310,16 +311,20 @@ def test_required_and_unique_tags_are_checked_only_on_assembled_rows(self): issues = self.validator.validate(ListColumnSource({"condition": ["a"]}), sidecar=repeated) self.assertEqual([issue["code"] for issue in issues], [ValidationErrors.TAG_NOT_UNIQUE]) - # The same holds for a unique tag repeated within one cell. + # A unique tag repeated within one cell is reported without assembly, once with a row count, and + # with assembly once per row; never both, since the cell pass runs only when no assembly follows. columns = { - "onset": [1.0], - "duration": [0], - "HED": ["(Event-context, (Red, Blue)), (Event-context, (Green, Yellow))"], + "onset": [1.0, 2.0], + "duration": [0, 0], + "HED": ["(Event-context, (Red, Blue)), (Event-context, (Green, Yellow))"] * 2, } - self.assertEqual(self.validator.validate(ListColumnSource(columns)), []) + issues = self.validator.validate(ListColumnSource(columns)) + self.assertEqual([issue["code"] for issue in issues], [ValidationErrors.TAG_NOT_UNIQUE]) + self.assertEqual((issues[0][ErrorContext.ROW], issues[0][ROW_COUNT_KEY]), (0, 2)) frame = TabularInput(pd.DataFrame({name: [str(v) for v in values] for name, values in columns.items()})) issues = self.validator.validate(ListColumnSource(columns, base_input=frame)) - self.assertEqual([issue["code"] for issue in issues], [ValidationErrors.TAG_NOT_UNIQUE]) + self.assertEqual([issue["code"] for issue in issues], [ValidationErrors.TAG_NOT_UNIQUE] * 2) + self.assertTrue(all(ROW_COUNT_KEY not in issue for issue in issues)) def test_plain_source_gets_a_warning_for_a_column_the_sidecar_does_not_describe(self): source = ListColumnSource({"id": [1, 2], "onset": [1.0, 2.0], "HED": ["Red", "Blue"]})