Skip to content

Undescriptive error message for undefined group #623

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

Closed
RubelMozumder wants to merge 12 commits into from
Closed

Undescriptive error message for undefined group #623

Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ff1506e
remove unused import.
RubelMozumder Apr 10, 2025
d9218ff
Add proper warning massage for incorrect group symantic.
RubelMozumder Apr 10, 2025
3ed3e57
update the warning massage.
RubelMozumder Apr 10, 2025
15d7217
Tests are passing.
RubelMozumder Apr 13, 2025
b4166c5
Polish warning massage.
RubelMozumder Apr 14, 2025
315664e
Remove the check.
RubelMozumder Apr 14, 2025
26bbd0c
Remove uncessary variable.
RubelMozumder Apr 14, 2025
f53a43f
convert to warning.
RubelMozumder Apr 14, 2025
b94e99d
Add file variable and use get instead of try block.
RubelMozumder Apr 14, 2025
4ede930
Tiny update.
RubelMozumder Apr 15, 2025
907cf01
typo
RubelMozumder Apr 15, 2025
40dbe2f
Warning massage.
RubelMozumder Apr 25, 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
116 changes: 58 additions & 58 deletions src/pynxtools/dataconverter/writer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

import copy
import logging
import sys
import xml.etree.ElementTree as ET

import h5py
Expand Down Expand Up @@ -117,7 +116,9 @@ def handle_dicts_entries(data, grp, entry_name, output_path, path):
- Concatenate dataset in one virtual dataset
- Internal links
- External links
- compression label"""
- compression label
"""
file = None
if "link" in data:
file, path = split_link(data, output_path)
# generate virtual datasets from slices
Expand Down Expand Up @@ -246,11 +247,19 @@ def ensure_and_get_parent_node(self, path: str, undocumented_paths) -> h5py.Grou
if not does_path_exist(parent_path, self.output_nexus):
parent = self.ensure_and_get_parent_node(parent_path, undocumented_paths)
grp = parent.create_group(parent_path_hdf5)

attrs = self.__nxdl_to_attrs(parent_path)
if attrs is not None and (nx_class := attrs.get("type", "")):
grp.attrs["NX_class"] = nx_class
else:
logger.warning(
"NXDL attribute `type` not found for group %s.\n"
"Hint: Follow the convention `fixednameVARIADICPART[fixedname_given_name]` "
"or `non_variadic_group` where `fixednameVARIADICPART` is a variadic name "
"and `non_variadic_group` is a fixed name of the group defined in NXDL file "
"Or no such group exists in NXDL file.",
parent_path,
)
Comment on lines +255 to +261

@lukaspie lukaspie Apr 25, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I still don't think that error message makes that much sense. The first case (i.e., not using fixednameVARIADICPART[fixedname_given_name] for a concept that has a renameable part) is now fixed by #621, so I don't think we will get to this point if you don't use it properly. Thus, that part of the error message can be removed. I think if you rebase on top of master, such a problem will not occur here anymore.

The actual issue is if we try to attach a field for a group that doesn't exist, but for which the same name is used as a field. For example, using "/ENTRY/entry_identifier/identifier" as a key. entry_identifier is a field with XML attribute deprecated in NXentry, but with this notation we are using it as if it were a named group. Obviously, a field cannot have a sub-field itself, so that will lead to problems. We should instead raise an error that we are trying to write a group with a name that is reserved for a field instead.

This is something that should be caught by the validation instead of in the writer IMO.

@lukaspie lukaspie Apr 25, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should not allow giving a group some name that is defined as the name of an field/attribute on the same level in general. This may be technically allowed in NeXus, but will break the whole NOMAD integration (since groups are (sub)sections and fields are quantities).

@RubelMozumder RubelMozumder May 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we can remove the error massage 👍, as this message mainly says the invalid group or misdefined group. This sort of wanrinig is part of the validation. Here, writer will write the nexus file, if anything is wrong with the writing process that should be handeled here. To check a group, a field or attribute has proper annotation is not part of the writer.

However, in a point I am a bit confused that is raising error wrting a group with the same name of attribute or field (which may come from mistyping, or just a regular error). In that case, group would not get proper nx_class name in such case NOMAD just will overlook that group in registering. Because, nomad can not match that group with any of the subsection in nomad metainfo.

@rettigl rettigl May 2, 2025 •
edited
Loading

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.

We should not allow giving a group some name that is defined as the name of an field/attribute on the same level in general. This may be technically allowed in NeXus, but will break the whole NOMAD integration (since groups are (sub)sections and fields are quantities).

I agree, this creates the whole problem in the first place. We could consider to allow it to pass, but raise a strong and descriptive warning message.
With the current master, this still gets through validation with
WARNING: Field /ENTRY[entry]/entry_identifier/identifier written without documentation.
And no hint to the conflicting concept, and then breaks in the writer.

I would suggest to do three things:

  • Add a check for conflicting concepts to the validation with clear and descriptive warning messages
  • Catch any issue this might create in the writer (consider e.g. also a field of a name being defined, and the same name also as a group), also with clear and descriptive error messages
  • Any other changes to the code that don't relate to this specific problem I suggest to move to a different PR. It's very hard to review this way, because most changes you made don't relate to the problem as far as I can tell.

@rettigl rettigl May 2, 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.

In the current master, you can actually provide something like:

  "/ENTRY/identifier_entry": "@attrs:metadata/loader/scan_path",
  "/ENTRY/COLLECTION[entry_identifier]": {
    "identifier":"@attrs:metadata/loader/scan_path"
  },

And it breaks writing, but in a different fashion:

WARNING: Given field name 'COLLECTION' conflicts with the non-variadic name 'entry_identifier (opt)'
WARNING: Field /ENTRY[entry]/COLLECTION[entry_identifier]/identifier written without documentation.


---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
File /mnt/pcshare/users/Laurenz/AreaB/transformations_testing/pynxtools/src/pynxtools/dataconverter/writer.py:294, in Writer._put_data_into_hdf5(self)
    293         else:
--> 294             dataset = grp.create_dataset(entry_name, data=data)
    295 except InvalidDictProvided as exc:

File /mnt/pcshare/users/Laurenz/AreaB/transformations_testing/.venv/lib/python3.10/site-packages/h5py/_hl/group.py:183, in Group.create_dataset(self, name, shape, dtype, data, **kwds)
    181         group = self.require_group(parent_path)
--> 183 dsid = dataset.make_new_dset(group, shape, dtype, data, name, **kwds)
    184 dset = dataset.Dataset(dsid)

File /mnt/pcshare/users/Laurenz/AreaB/transformations_testing/.venv/lib/python3.10/site-packages/h5py/_hl/dataset.py:165, in make_new_dset(parent, shape, dtype, data, name, chunks, compression, shuffle, fletcher32, maxshape, compression_opts, fillvalue, scaleoffset, track_times, external, track_order, dcpl, dapl, efile_prefix, virtual_prefix, allow_unknown_filter, rdcc_nslots, rdcc_nbytes, rdcc_w0, fill_time)
    163     sid = h5s.create_simple(shape, maxshape)
--> 165 dset_id = h5d.create(parent.id, name, tid, sid, dcpl=dcpl, dapl=dapl)
    167 if (data is not None) and (not isinstance(data, Empty)):

File h5py/_objects.pyx:54, in h5py._objects.with_phil.wrapper()

File h5py/_objects.pyx:55, in h5py._objects.with_phil.wrapper()

File h5py/h5d.pyx:136, in h5py.h5d.create()

ValueError: Unable to synchronously create dataset (name already exists)

The above exception was the direct cause of the following exception:

OSError                                   Traceback (most recent call last)
Cell In[17], line 9
      2 res = sps.load_scan(
      3     scan=scan,
      4     metadata=metadata,
      5     collect_metadata=True,
      6     # iterations=[0, 3, 6],
      7 )
      8 # Save scan
----> 9 sps.save(f"Scan{scan}.nxs")
     10 # Some plots
     11 if len(res.dims) == 2:

File /mnt/pcshare/users/Laurenz/AreaB/transformations_testing/specsanalyzer/src/specsscan/core.py:668, in SpecsScan.save(self, faddr, **kwds)
    665     if "eln_data" in kwds:
    666         input_files.append(kwds.pop("eln_data"))
--> 668     to_nexus(
    669         data=self._result,
    670         faddr=faddr,
    671         reader=reader,
    672         definition=definition,
    673         input_files=input_files,
    674         **kwds,
    675     )
    677 else:
    678     raise NotImplementedError(
    679         f"Unrecognized file format: {extension}.",
    680     )

File /mnt/pcshare/users/Laurenz/AreaB/transformations_testing/specsanalyzer/src/specsanalyzer/io.py:426, in to_nexus(data, faddr, reader, definition, input_files, **kwds)
    423 else:
    424     input_files = tuple(input_files)
--> 426 convert(
    427     input_file=input_files,
    428     objects=(data),
    429     reader=reader,
    430     nxdl=definition,
    431     output=faddr,
    432     **kwds,
    433 )

File /mnt/pcshare/users/Laurenz/AreaB/transformations_testing/pynxtools/src/pynxtools/dataconverter/convert.py:246, in convert(input_file, reader, nxdl, output, skip_verify, **kwargs)
    236 data = transfer_data_into_template(
    237     input_file=input_file,
    238     reader=reader,
   (...)
    242     **kwargs,
    243 )
    245 helpers.add_default_root_attributes(data=data, filename=os.path.basename(output))
--> 246 Writer(data=data, nxdl_f_path=nxdl_f_path, output_path=output).write()
    248 logger.info(f"The output file generated: {output}.")

File /mnt/pcshare/users/Laurenz/AreaB/transformations_testing/pynxtools/src/pynxtools/dataconverter/writer.py:342, in Writer.write(self)
    340 """Writes the NeXus file with previously validated data from the reader with NXDL attrs."""
    341 try:
--> 342     self._put_data_into_hdf5()
    343 finally:
    344     self.output_nexus.close()

File /mnt/pcshare/users/Laurenz/AreaB/transformations_testing/pynxtools/src/pynxtools/dataconverter/writer.py:298, in Writer._put_data_into_hdf5(self)
    296         print(str(exc))
    297     except Exception as exc:
--> 298         raise IOError(
    299             f"Unknown error occured writing the path: {path} "
    300             f"with the following message: {str(exc)}"
    301         ) from exc
    303 for links in hdf5_links_for_later:
    304     dataset = handle_dicts_entries(*links)

OSError: Unknown error occured writing the path: /ENTRY[entry]/entry_identifier with the following message: Unable to synchronously create dataset (name already exists)

@lukaspie lukaspie May 5, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

WARNING: Given field name 'COLLECTION' conflicts with the non-variadic name 'entry_identifier (opt)'

This is already correctly throwing a warning (should be an error though), but it should probably tell you that one is a field (defined and deprcated in `NXentry´) and the other is a new group.

@RubelMozumder RubelMozumder May 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We should not allow giving a group some name that is defined as the name of an field/attribute on the same level in general. This may be technically allowed in NeXus, but will break the whole NOMAD integration (since groups are (sub)sections and fields are quantities).

I agree, this creates the whole problem in the first place. We could consider to allow it to pass, but raise a strong and descriptive warning message. With the current master, this still gets through validation with WARNING: Field /ENTRY[entry]/entry_identifier/identifier written without documentation. And no hint to the conflicting concept, and then breaks in the writer.

I would suggest to do three things:

  • Add a check for conflicting concepts to the validation with clear and descriptive warning messages
  • Catch any issue this might create in the writer (consider e.g. also a field of a name being defined, and the same name also as a group), also with clear and descriptive error messages
  • Any other changes to the code that don't relate to this specific problem I suggest to move to a different PR. It's very hard to review this way, because most changes you made don't relate to the problem as far as I can tell.

Yeah, I can create two PRs referring to this PR.

I think both of the warnings are properly described the issue here.
E.g., the warning WARNING: Field /ENTRY[entry]/entry_identifier/identifier written without documentation says the correct warning. Indeed, there is not such group entry_identifier and even no such field.

@lukaspie lukaspie May 5, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should not allow this in the first place if there is also a field called entry_identifier defined at the same level.


if attrs is not None:
grp.attrs["NX_class"] = attrs["type"]
return grp
return self.output_nexus[parent_path_hdf5]

Expand All @@ -259,82 +268,73 @@ def _put_data_into_hdf5(self):

hdf5_links_for_later = []

def add_units_key(dataset, path):
units_key = f"{path}/@units"
if units_key in self.data.keys() and self.data[units_key] is not None:
dataset.attrs["units"] = self.data[units_key]
unit_and_attr = {}

for path, value in self.data.items():
for path, d_value in self.data.items():
if not is_not_data_empty(d_value):
continue
try:
if path[path.rindex("/") + 1 :] == "@units":
continue

entry_name = helpers.get_name_from_data_dict_entry(
path[path.rindex("/") + 1 :]
)
if is_not_data_empty(value):
data = value
else:
if entry_name and entry_name.startswith("@"):
unit_and_attr[path] = d_value
continue

if entry_name[0] != "@":
grp = self.ensure_and_get_parent_node(
path, self.data.undocumented.keys()
)
if isinstance(data, dict):
if "compress" in data.keys():
dataset = handle_dicts_entries(
data, grp, entry_name, self.output_path, path
)
else:
hdf5_links_for_later.append(
[data, grp, entry_name, self.output_path, path]
)
# Handle fields and groups
grp = self.ensure_and_get_parent_node(
path,
self.data.undocumented.keys(),
)
if isinstance(d_value, dict):
if "compress" in d_value.keys():
dataset = handle_dicts_entries(
d_value, grp, entry_name, self.output_path, path
)
else:
dataset = grp.create_dataset(entry_name, data=data)
hdf5_links_for_later.append(
[d_value, grp, entry_name, self.output_path, path]
)
else:
dataset = grp.create_dataset(entry_name, data=d_value)

except InvalidDictProvided as exc:
print(str(exc))
except Exception as exc:
raise IOError(
f"Unknown error occured writing the path: {path} "
f"with the following message: {str(exc)}"
f"with the following message: {str(exc)}\n"
) from exc

# Handle links
for links in hdf5_links_for_later:
dataset = handle_dicts_entries(*links)
if dataset is None:
# If target of a link is invalid to be linked
del self.data[links[-1]]

for path, value in self.data.items():
# Handle units and attributes
for path, d_value in unit_and_attr.items():
hdf5_path = helpers.convert_data_dict_path_to_hdf5_path(path)
entry_name = helpers.get_name_from_data_dict_entry(
path[path.rindex("/") + 1 :]
)
try:
if path[path.rindex("/") + 1 :] == "@units":
continue

entry_name = helpers.get_name_from_data_dict_entry(
path[path.rindex("/") + 1 :]
dataset_or_grp = self.ensure_and_get_parent_node(
path,
self.data.undocumented.keys(),
)
if is_not_data_empty(value):
data = value
else:
continue

if entry_name[0] != "@":
path_hdf5 = helpers.convert_data_dict_path_to_hdf5_path(path)

add_units_key(self.output_nexus[path_hdf5], path)
else:
# consider changing the name here the lvalue can also be group!
dataset = self.ensure_and_get_parent_node(
path, self.data.undocumented.keys()
)
dataset.attrs[entry_name[1:]] = data
except Exception as exc:
raise IOError(
f"Unknown error occured writing the path: {path}"
f", while writing the value: {value} "
f"with the following message: {str(exc)}"
) from exc
except KeyError as exc:
logger.warning(
"No path '%s' available to attached Attribute.", hdf5_path
)
dataset_or_grp = None
if dataset_or_grp is None:
continue
if path.endswith("/@units"):
dataset_or_grp.attrs["units"] = d_value
else:
dataset_or_grp.attrs[entry_name[1:]] = d_value

@rettigl rettigl Apr 14, 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.

What issue do these changes solve? I don't understand it.

@RubelMozumder RubelMozumder Apr 14, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does not solve anything. This separates the concept handling of group and field from that of attributes and units.
I also mentioned that in the PR description.

@lukaspie lukaspie Apr 25, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do they need to be separated? What do we gain from this? If we have so many changes to the code, it would be good to understand the reasoning for why these changes are made.


def write(self):
"""Writes the NeXus file with previously validated data from the reader with NXDL attrs."""
Expand Down