Skip to content

Error for multiple different variadic concepts with the same name #646

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lukaspie merged 10 commits into from
Jun 5, 2025
Merged

Error for multiple different variadic concepts with the same name #646

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2e27912
log error if there re multiple different variadic concepts with the s…
lukaspie May 26, 2025
eaba076
fix for groups that are not on the same level
lukaspie May 26, 2025
29f290e
remove unneeded enumeration
lukaspie May 27, 2025
d499cc6
continue if keys has no variable concepts
lukaspie May 27, 2025
0225333
use existing docs style
lukaspie May 28, 2025
40295fa
only remove conflicting keys if each of them is valid
lukaspie May 28, 2025
8d0e62b
Apply suggestions from code review
lukaspie Jun 5, 2025
aacf70e
use keys_to_remove from global context
lukaspie Jun 5, 2025
6daffdc
catch case where there are multiple fields in the conflicting groups
lukaspie Jun 5, 2025
14f9562
catch case where a key with a concept name has an equivalent non-vari…
lukaspie Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/pynxtools/dataconverter/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@


class ValidationProblem(Enum):
DifferentVariadicNodesWithTheSameName = auto()
UnitWithoutDocumentation = auto()
InvalidEnum = auto()
OpenEnumWithNewItem = auto()
Expand Down Expand Up @@ -85,6 +86,13 @@ def _log(self, path: str, log_type: ValidationProblem, value: Optional[Any], *ar
if value is None:
value = "<unknown>"

if log_type == ValidationProblem.DifferentVariadicNodesWithTheSameName:
value = cast(Any, value)
logger.warning(
f"Instance name '{path}' used for multiple different concepts: "
f"{', '.join(sorted(set(c for c, _ in value)))}. "
f"The following keys are affected: {', '.join(sorted(set(k for _, k in value)))}."
)
if log_type == ValidationProblem.UnitWithoutDocumentation:
logger.info(
f"The unit, {path} = {value}, is being written but has no documentation."
Expand Down
91 changes: 90 additions & 1 deletion src/pynxtools/dataconverter/validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -850,6 +850,94 @@ def recurse_tree(

handling_map.get(child.type, handle_unknown_type)(child, keys, prev_path)

def find_instance_name_conflicts(
mapping: MutableMapping[str, str], keys_to_remove: List[str]
) -> None:
"""
Detect and log conflicts where the same variadic instance name is reused across
different concept names.

This function ensures that a given instance name (e.g., 'my_name') is only used
for a single concept (e.g., SAMPLE or USER, but not both). Reusing the same instance
name for different concept names (e.g., SAMPLE[my_name] and USER[my_name]) is
considered a conflict.

For example, this is a conflict:
/ENTRY[entry1]/SAMPLE[my_name]/...
/ENTRY[entry1]/USER[my_name]/...

But this is NOT a conflict:
/ENTRY[entry1]/INSTRUMENT[instrument]/FIELD[my_name]/...
/ENTRY[entry1]/INSTRUMENT[instrument]/DETECTOR[detector]/FIELD[my_name]/...

When such conflicts are found, an error is logged indicating the instance name
and the conflicting concept names. Additionally, all keys involved in the conflict
are logged and added to the `keys_to_remove` list.

Args:
mapping (MutableMapping[str, str]):
The mapping containing the data to validate.
This should be a dict of `/` separated paths, such as
"/ENTRY[entry1]/SAMPLE[sample1]/name".
keys_to_remove (List[str]):
List of keys that will be removed from the template. This is extended here
in the case of conflicts.

"""
pattern = re.compile(r"(?P<concept_name>[^\[\]/]+)\[(?P<instance>[^\]]+)\]")

# Tracks instance usage with respect to their parent group
instance_usage: Dict[Tuple[str, str], List[Tuple[str, str]]] = defaultdict(list)

for key in mapping:
matches = list(pattern.finditer(key))
Comment thread
lukaspie marked this conversation as resolved.
if not matches:
# The keys contains no concepts with variable name, no need to further check
continue
for match in matches:
concept_name = match.group("concept_name")
instance_name = match.group("instance")

# Determine the parent path up to just before this match
parent_path = key[: match.start()]
instance_usage[(instance_name, parent_path)].append((concept_name, key))

for (instance_name, parent_path), entries in sorted(instance_usage.items()):
concept_names = {c for c, _ in entries}
if len(concept_names) > 1:
keys = sorted(k for _, k in entries)
collector.collect_and_log(
instance_name,
ValidationProblem.DifferentVariadicNodesWithTheSameName,
entries,
)
# Now that we have name conflicts, we still need to check that there are
# at least two valid keys in that conclit. Only then we remove these.
Comment thread
lukaspie marked this conversation as resolved.
Outdated
# This takes care of the example with keys like
# /ENTRY[my_entry]/USER[some_name]/name and /ENTRY[my_entry]/USERS[some_name]/name,
# where we only want to keep the first one.
valid_keys_with_name_conflicts = []

for key in keys:
try:
node = add_best_matches_for(key, tree)
if node is not None:
valid_keys_with_name_conflicts.append(key)
continue
except TypeError:
pass
Comment on lines +949 to +950

@rettigl rettigl May 28, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When do we arrive here?

@lukaspie lukaspie Jun 5, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is from here, which is used here to remove keys where we have an invalid type for a named concept. It was introduced in #638.

Not the most elegant solution, but works for now. We should probably not even reach the TypeError here, but at least we are covered.

collector.collect_and_log(
key, ValidationProblem.KeyToBeRemoved, "key"
)
keys_to_remove.append(key)

if len(valid_keys_with_name_conflicts) > 1:
for valid_key in valid_keys_with_name_conflicts:
collector.collect_and_log(
valid_key, ValidationProblem.KeyToBeRemoved, "key"
)
keys_to_remove.append(valid_key)

def check_attributes_of_nonexisting_field(
node: NexusNode,
):
Expand Down Expand Up @@ -1174,10 +1262,11 @@ def check_reserved_prefix(
"choice": handle_choice,
}

keys_to_remove = []
keys_to_remove: List[str] = []

tree = generate_tree_from(appdef)
collector.clear()
find_instance_name_conflicts(mapping, keys_to_remove)

@rettigl rettigl May 28, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the functions below, you don't pass the keys_to_remove, but use them from the global context. Why is this handled differently here?

@lukaspie lukaspie Jun 5, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the function somewhere else first, where the global context was not available. Removed it now.

nested_keys = build_nested_dict_from(mapping)
not_visited = list(mapping)
keys = _follow_link(nested_keys, "")
Expand Down
57 changes: 55 additions & 2 deletions tests/dataconverter/test_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,59 @@ def listify_template(data_dict: Template):
@pytest.mark.parametrize(
"data_dict,error_messages",
[
pytest.param(
alter_dict(
alter_dict(
alter_dict(
alter_dict(
alter_dict(
alter_dict(
alter_dict(
alter_dict(
TEMPLATE,
"/ENTRY[my_entry]/SAMPLE[some_name]/name",
"A sample name",
),
"/ENTRY[my_entry]/USER[some_name]/name",
"A user name",
),
"/ENTRY[my_entry]/MONITOR[some_name]/name",
"An monitor name",
),
"/ENTRY[my_entry]/INSTRUMENT[instrument]/APERTURE[another_name]/name",
"An aperture within an instrument",
),
"/ENTRY[my_entry]/INSTRUMENT[instrument]/DETECTOR[another_name]/name",
"A detector within an instrument",
),
"/ENTRY[my_entry]/INSTRUMENT[instrument]/SOURCE[my_source]/APERTURE[another_name]/name",
"An aperture within a source inside an instrument",
),
"/ENTRY[my_entry]/USER[a_third_name]/name",
"A tird user name",
),
"/ENTRY[my_entry]/USERS[a_third_name]/name",
"An invalid group of the same name",
),
[
"Instance name 'a_third_name' used for multiple different concepts: USER, USERS. "
"The following keys are affected: /ENTRY[my_entry]/USERS[a_third_name]/name, "
"/ENTRY[my_entry]/USER[a_third_name]/name.",
"The key /ENTRY[my_entry]/USERS[a_third_name]/name will not be written.",
"Instance name 'another_name' used for multiple different concepts: APERTURE, DETECTOR. "
"The following keys are affected: /ENTRY[my_entry]/INSTRUMENT[instrument]/APERTURE[another_name]/name, "
"/ENTRY[my_entry]/INSTRUMENT[instrument]/DETECTOR[another_name]/name.",
"The key /ENTRY[my_entry]/INSTRUMENT[instrument]/APERTURE[another_name]/name will not be written.",
"The key /ENTRY[my_entry]/INSTRUMENT[instrument]/DETECTOR[another_name]/name will not be written.",
"Instance name 'some_name' used for multiple different concepts: MONITOR, SAMPLE, USER. "
"The following keys are affected: /ENTRY[my_entry]/MONITOR[some_name]/name, "
"/ENTRY[my_entry]/SAMPLE[some_name]/name, /ENTRY[my_entry]/USER[some_name]/name.",
"The key /ENTRY[my_entry]/MONITOR[some_name]/name will not be written.",
"The key /ENTRY[my_entry]/SAMPLE[some_name]/name will not be written.",
"The key /ENTRY[my_entry]/USER[some_name]/name will not be written.",
],
id="variadic-groups-of-the-same-name",
),
pytest.param(
alter_dict(
alter_dict(
Expand Down Expand Up @@ -1079,11 +1132,11 @@ def listify_template(data_dict: Template):
pytest.param(
alter_dict(
TEMPLATE,
"/ENTRY[my_entry]/INSTRUMENT[my_instrument]/ILLEGAL[my_source]/type",
"/ENTRY[my_entry]/INSTRUMENT[my_instrument]/ILLEGAL[my_source2]/type",
1,
),
[
"Field /ENTRY[my_entry]/INSTRUMENT[my_instrument]/ILLEGAL[my_source]/type written without documentation."
"Field /ENTRY[my_entry]/INSTRUMENT[my_instrument]/ILLEGAL[my_source2]/type written without documentation."
],
id="bad-namefitting",
),
Expand Down