Optimise depends_on chain resolution by using h5py by jl-wynen · Pull Request #253 · scipp/scippnexus

jl-wynen · 2024-11-19T10:54:58Z

This reduces the time it takes to resolve depends_on chains by locating the target using h5py and a custom path resolver. It avoids creating intermediate snx.Group objects which is expensive.

For loading the 45 BIFROST analysers using

def load(fname):
    start = time.perf_counter()
    with snx.File(fname) as f:
        instrument = f['entry']['instrument']
        groups = instrument[snx.NXcrystal]
        start_load = time.perf_counter()
        crystals = {k: v[()] for k, v in groups.items()}
        end_load = time.perf_counter()
    closed = time.perf_counter()
    print(f'total: {closed-start:.3f}s   load: {end_load-start_load:.3f}s')

I get these results on my machine:

# Baseline:       total: 1.205s   load: 1.082s
# Custom resolve: total: 0.345s   load: 0.306s

SimonHeybrock · 2024-11-19T11:09:42Z

        while depends_on != '.':
-            transform = parent[depends_on]
+            transform = _locate_depends_on_target(parent, depends_on)
            parent = transform.parent


Have not looked at the rest of the code yet, but can't you do more here? The parent logic is still stepping back and forth between h5py and ScippNexus, and I don't know if transform.parent comes "for free". Can't parent be an h5py.Group for the entire loop?

I did this now. But it doesn't affect performance.

jl-wynen · 2024-11-20T08:11:11Z

+            self.sizes = _squeezed_field_sizes(self.dataset)
+        if self.dtype is None:
+            self.dtype = _dtype_fromdataset(self.dataset)
+


Please check that this is sound!

I feared that there might be some code that relies on field.sizes being None (e.g., to do custom init logic only if dims are not set yet) before calling NXobject.__init__, but I could not find any (looking mainly at nxdata.py). So I think it should be ok?

SimonHeybrock · 2024-11-20T08:59:30Z

        """Subclasses should call this in their __init__ method, or ensure that they
        initialize the fields in `children` with the correct sizes and dtypes."""


The docstring is outdated now, at least in parts?

SimonHeybrock · 2024-11-20T09:01:05Z

+            self.sizes = _squeezed_field_sizes(self.dataset)
+        if self.dtype is None:
+            self.dtype = _dtype_fromdataset(self.dataset)
+


I feared that there might be some code that relies on field.sizes being None (e.g., to do custom init logic only if dims are not set yet) before calling NXobject.__init__, but I could not find any (looking mainly at nxdata.py). So I think it should be ok?

SimonHeybrock · 2024-11-20T09:02:02Z

+    base_parts = list(cwd.parts)
+    path_parts = [segment for segment in path.parts if segment != '.']
+    while path_parts[0] == '..':
+        if not path_parts:
+            raise ValueError(f"Relative path beyond root: '{path}'")
+        base_parts.pop(-1)
+        path_parts.pop(0)
+    return PurePosixPath(*base_parts, *path_parts)


Can we use something like

scippnexus/src/scippnexus/field.py

Lines 39 to 42 in 349c8df

def absolute_path(self) -> str | None:

if self.value == '.':

return None

return posixpath.normpath(posixpath.join(self.parent, self.value))

?

What is posixpath?

import posixpath

SimonHeybrock · 2024-11-20T09:02:50Z

+
+    The returned object is equivalent to calling ``parent[depends_on]``
+    in the context of transformations.
+    This function does not work in general because it does not handle NXdata attributes.


Not sure why you mention NXdata attributes? What are you referring to?

I thought this is what the comment here

scippnexus/src/scippnexus/base.py

Lines 371 to 373 in 349c8df

# We cannot get the child directly from the HDF5 group, since we need to

# create the parent group, to ensure that fields get the correct properties

# such as sizes and dtype.

refers to.

Yes, but NXdata is just one case (it is a baseclass for some but not all other NX* implementations).

SimonHeybrock · 2024-11-20T09:07:09Z

+            transform, base_path = _locate_depends_on_target(
+                file, base_path, PurePosixPath(depends_on), parent.definitions
+            )
            depends_on = transform.attrs['depends_on']


As we are loading the transform anyway, and the loaded Transform has access to its parent via its depends_on, do we need the base_path returned from _locate_depends_on_target? It seems like a duplicate control flow, unless I am missing something.

I did this to avoid accessing the parent because of #253 (comment)

My point was: We are loading the transform anyway (in the line below), so the parent path is just a Python string stored in a Python class? There should be no more snx.Group stuff at the level.

Where is it stored? Transform objects don't track their parent path or any other path. And the object returned from _locate_depends_on_target is Field or Group. And accessing its parent gets us back to #253 (comment)

It is in Transform.depends_on.parent, or is that a different one?

I see. The DependsOn object could even make the whole implementation simpler.
But it doesn't actually work because transform[()] can return DataArray if the transform is broken in the file. In that case, the current code still builds a complete transformation chain that contains data arrays instead of Transform objects. But those don't carry a depends_on attribute. See, e.g., test_compute_transformation_warns_if_transformation_missing_vector_attr.

So I don't see an alternative to tracking the parent path manually here that doesn't change the behaviour.

Custom group resolution for depends_on

a4ab7d1

SimonHeybrock reviewed Nov 19, 2024

View reviewed changes

jl-wynen added 2 commits November 20, 2024 09:10

Operate entirely on h5py objects

44ebeb6

Init field within Field class

f72aff4

jl-wynen force-pushed the opt-depends_on-resolution branch from b9e15b7 to f72aff4 Compare November 20, 2024 08:10

jl-wynen commented Nov 20, 2024

View reviewed changes

SimonHeybrock reviewed Nov 20, 2024

View reviewed changes

Use DependsOn directly

2138887

SimonHeybrock approved these changes Nov 25, 2024

View reviewed changes

jl-wynen merged commit ec20200 into main Nov 25, 2024

jl-wynen deleted the opt-depends_on-resolution branch November 25, 2024 10:22

This was referenced Jan 17, 2025

Speed regression #245

Closed

Closing files takes a long* time #252

Closed

		"""Subclasses should call this in their __init__ method, or ensure that they
		initialize the fields in `children` with the correct sizes and dtypes."""

	def absolute_path(self) -> str \| None:
	if self.value == '.':
	return None
	return posixpath.normpath(posixpath.join(self.parent, self.value))

	# We cannot get the child directly from the HDF5 group, since we need to
	# create the parent group, to ensure that fields get the correct properties
	# such as sizes and dtype.

Conversation

jl-wynen commented Nov 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants