when getting object by address do not fully instantiate when iterating by kmuehlbauer · Pull Request #83 · NCAS-CMS/pyfive

kmuehlbauer · 2025-08-15T12:31:57Z

Description

The changes in this PR prevents instantiating objects fully when retrieving objects by address (eg. when dereferencing). This was reported in #70 (comment).

Checklist

This pull request has a descriptive title and labels
This pull request has a minimal description (most was discussed in the issue, but a two-liner description is still desirable)
Unit tests have been added (if codecov test fails)
Any changed dependencies have been added or removed correctly (if need be)
If you are working on the documentation, please ensure the current build passes
All tests pass

valeriupredoi · 2025-08-19T13:22:39Z

thanks a lot @kmuehlbauer - mind fixing the test too, please, mate? @bnlawrence you got a minute have a look at Kai's proposed change pls? 🍺

kmuehlbauer · 2025-08-19T13:36:43Z

@valeriupredoi I'm really sure there is a better solution to this here. I've only did this branch to let you know, what I'm about to. I've more details in #70 (comment).

The main problem is, that pyfive raises NotImplemented when a not implemented feature get's instanciated/requested. But, this also happens when dereferencing a Reference using visititems, which will instanciate all variables (including any not implemented) and raise. I can't do anything to prevent or work around from the h5netcdf-side. I hope @bnlawrence can spot the issue and maybe has already an idea to get around that.

bnlawrence · 2025-08-19T13:53:48Z

@valeriupredoi I'm really sure there is a better solution to this here. I've only did this branch to let you know, what I'm about to. I've more details in #70 (comment).

The main problem is, that pyfive raises NotImplemented when a not implemented feature get's instanciated/requested. But, this also happens when dereferencing a Reference using visititems, which will instanciate all variables (including any not implemented) and raise. I can't do anything to prevent or work around from the h5netcdf-side. I hope @bnlawrence can spot the issue and maybe has already an idea to get around that.

I know @davidhassell was thinking about this too. Before I dive in, @davidhassell, did you think any more about it? (As it happens, I am dead sick of proposal spreadsheets, so thinking about this for an hour or two would be a blessed relief.)

valeriupredoi · 2025-08-19T14:24:30Z

good man @kmuehlbauer - I usually open a Draft PR when it comes to these types, take your time, and am sure Bryan, David can help - this one's a bit out of my reach but I can help with fixing its tests etc, when time comes 🍺

kmuehlbauer · 2025-08-19T14:26:49Z

@valeriupredoi Ah, sorry, this should have been a draft.

bnlawrence · 2025-08-19T14:47:21Z

thanks a lot @kmuehlbauer - mind fixing the test too, please, mate? @bnlawrence you got a minute have a look at Kai's proposed change pls? 🍺

Actually, it would be really useful to have a test (or tests) that shows the behaviour that is wanted here, which is presumably to fail when you want it to and not before.

bmaranville · 2025-08-19T15:30:07Z

I would agree that visititems is not needed (or appropriate?) for the dereference function.

Currently it's calling _get_obj_by_addr(self, obj_addr), which is then calling visititems, but I think you could just instantiate a new DataObjects with that offset and query it with is_dataset() to determine if it is a Group or Dataset, and then instantiate one of those to return, like is done here: https://github.com/NCAS-CMS/pyfive/blob/main/pyfive/high_level.py#L91

kmuehlbauer · 2025-08-19T16:22:15Z

@bmaranville Thanks for chiming in. This seems like a good way forward. I'll try to implement your suggestions here the next day. But, if someone has free cycles, feel free to add or override here.

bnlawrence · 2025-08-19T16:29:02Z

How about we start from deciding that the determine_dtype method of DataTypeMessageshould return something sensible for all dtypes, and we deal with the fact that pyfive doesn't implement them all elsewhere? Leaving aside whether or not we use visititems for dereferencing this would seem like a "good thing" (TM), wouldn't it? It kind of does now insofar as it tells you what it is as it raises itself in a heap ... but that's not as useful as saying, well, I can tell you what this is even if I can't read it?

kmuehlbauer · 2025-08-19T16:33:45Z

        elif datatype_class == DATATYPE_ENUMERATED:
-            raise NotImplementedError(
-                "Enumerated datatype class not supported.")
+            return ("ENUMERATED", datatype_msg['size'])


@bnlawrence So, would this be something useful to return?

Yes, I think so. The higher-level API for dtype will need to return uint8, but what we do in between is the key and I think we will need that information. We could check for that in the higher-level API, return uint8, and implement check_enum_dtype to look for this. We could quite quickly then at least implement the ability to look at the enumeration integers, even if we can't immediately decode the dictionary stored in the Enumeration Property Description.

I think DATATYPE_ENUMERATED can have a number of base types (any of the integer types?), not just uint8 - the base type is specified in the datatype message.

Quite right. Thanks for the reminder.

bnlawrence · 2025-08-20T07:36:44Z

I have added a new branch (enum_and_friends) where we can work on enum support. For the moment it is very dysfunctional, but it's a framework to work from, and includes a test which should make sure we support the h5py interface. Unfortunately I need to put this down to work on some other things and will be unlikely to follow all the way through before early September - but you never know, depends on progress and boredom levels with the other stuff I have to do right now. Ideally this branch also sorts out the visititems issue properly, but it might be that proceeding with this hack for the h5netcdf application is the right temporary expedient - but someone else will need to write the test for that.

bnlawrence · 2025-08-20T07:38:27Z

I have added a new branch (enum_and_friends) where we can work on enum support. For the moment it is very dysfunctional, but it's a framework to work from, and includes a test which should make sure we support the h5py interface. Unfortunately I need to put this down to work on some other things and will be unlikely to follow all the way through before early September - but you never know, depends on progress and boredom levels with the other stuff I have to do right now. Ideally this branch also sorts out the visititems issue properly, but it might be that proceeding with this hack for the h5netcdf application is the right temporary expedient - but someone else will need to write the test for that.

(And yes, I should have done it with a fork and a draft pull request, sorry, but I'm time poor right now and that would have taken me a wee bit longer to set up in VSC.)

kmuehlbauer · 2025-08-20T14:32:56Z

+        queue = deque([(self.name.rstrip('/'), self)])
+        while queue:
+            base, grp = queue.popleft()
+            for name, link_addr in grp._links.items():
+                full_path = f"{base}/{name}" if base else f"/{name}"
+                # check address without instantiating
+                if link_addr == obj_addr:
+                    # return instantiated object
+                    return grp[name]
+                # descend only if it's a subgroup (need to instantiate minimally)
+                # would be neat to have .is_group
+                dataobjs = DataObjects(self.file._fh, link_addr)
+                if not dataobjs.is_dataset and not dataobjs.is_datatype:
+                    queue.append((full_path, grp[name]))


This gets the object by address without instantiating completely. The DataObject creation is needed, to check for group and step into groups. Not sure if this more performant, but it doesn't instantiate the complete Datasets.

This reverts commit d37c080.

codecov · 2025-08-20T16:10:54Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.06%. Comparing base (7772b9b) to head (6f1224c).
⚠️ Report is 247 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #83      +/-   ##
==========================================
+ Coverage   71.93%   72.06%   +0.12%     
==========================================
  Files          11       11              
  Lines        2423     2434      +11     
  Branches      364      368       +4     
==========================================
+ Hits         1743     1754      +11     
+ Misses        583      582       -1     
- Partials       97       98       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

kmuehlbauer · 2025-08-20T17:18:26Z

OK, great, this seems to work now. I'll have to add tests.

kmuehlbauer · 2025-08-20T18:34:54Z

JFTR, as enum type is now handled in another PR, I'm concentrating here in the dereferencing of REFERENCE. This was done by using visititems which instantiated all objects which have been iterated over.

This now just compares the addresses and do a minimal test for group, where I added a respective is_group-method.

kmuehlbauer · 2025-08-20T20:51:11Z

Ready for review from my side. Thanks for all the suggestions and details.

kmuehlbauer · 2025-08-21T08:10:47Z

I was able to gain full coverage by adding another test. I'll leave this be now, so reviewers can have a look.

davidhassell · 2025-08-21T10:54:04Z

Thanks, @kmuehlbauer - I'm paying attention again now :), and will have a look at this PR today ...

davidhassell

Hi @kmuehlbauer,

Thank you for this change. The tests uses cases look good, the tests pass for me too. The new code makes sense, and I always like a corrected spelling mistake :).

We'll get this merged and out in a new PyPi release (0.5.1) today or tomorrow, so that it can be immediately used in the h5netcdf pyfive backend PR (h5netcdf/h5netcdf#273).

Cheers,
David

kmuehlbauer · 2025-08-21T14:08:55Z

Great news @davidhassell, happy to contribute.

davidhassell · 2025-08-21T14:17:17Z

(I'll be including this in the new Change Log that I'm about to start ...)

valeriupredoi · 2025-08-21T14:18:16Z

indeed, great many thanks @kmuehlbauer and @davidhassell 🍺 x2

hack: return None for not implemented things

d37c080

This was referenced Aug 15, 2025

NotImplementedError: Opaque datatype class not supported after last commit #70

Closed

Support pyfive as an alternative backend h5netcdf/h5netcdf#273

Closed

Support Pyfive as alternative backend, fix testsuite h5netcdf/h5netcdf#279

Merged

valeriupredoi requested a review from bnlawrence August 19, 2025 13:20

valeriupredoi added the TLC label Aug 19, 2025

kmuehlbauer commented Aug 19, 2025

View reviewed changes

bnlawrence mentioned this pull request Aug 19, 2025

Support Enumerated Data Types #85

Closed

dereference without instantiating

09df704

kmuehlbauer commented Aug 20, 2025

View reviewed changes

kmuehlbauer marked this pull request as draft August 20, 2025 15:59

Revert "hack: return None for not implemented things"

07d32a8

This reverts commit d37c080.

kmuehlbauer changed the title ~~hack: return None for not implemented~~ when getting object by address do not fully instantiate when iterating Aug 20, 2025

add is_group and use it

b279a34

kmuehlbauer added 2 commits August 20, 2025 22:41

clean-up

76c20ae

test dereferencing

c8c9b00

kmuehlbauer marked this pull request as ready for review August 20, 2025 20:50

kmuehlbauer added 2 commits August 21, 2025 09:57

add proper test for equality

cbc661d

check reference in subgroup

6f1224c

davidhassell approved these changes Aug 21, 2025

View reviewed changes

davidhassell merged commit a2d8433 into NCAS-CMS:main Aug 21, 2025
6 checks passed

kmuehlbauer deleted the handle-not-implemented branch August 21, 2025 14:18

kmuehlbauer mentioned this pull request Oct 2, 2025

Pyfive integration as backend into h5netcdf #113

Closed

10 tasks

Uh oh!

Conversation

kmuehlbauer commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

valeriupredoi commented Aug 19, 2025

Uh oh!

kmuehlbauer commented Aug 19, 2025

Uh oh!

bnlawrence commented Aug 19, 2025

Uh oh!

valeriupredoi commented Aug 19, 2025

Uh oh!

kmuehlbauer commented Aug 19, 2025

Uh oh!

bnlawrence commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmaranville commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kmuehlbauer commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnlawrence commented Aug 19, 2025

Uh oh!

kmuehlbauer Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

bnlawrence Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

bmaranville Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

bnlawrence Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

bnlawrence commented Aug 20, 2025

Uh oh!

bnlawrence commented Aug 20, 2025

Uh oh!

kmuehlbauer Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kmuehlbauer commented Aug 20, 2025

Uh oh!

kmuehlbauer commented Aug 20, 2025

Uh oh!

kmuehlbauer commented Aug 20, 2025

Uh oh!

kmuehlbauer commented Aug 21, 2025

Uh oh!

davidhassell commented Aug 21, 2025

Uh oh!

davidhassell left a comment

Choose a reason for hiding this comment

Uh oh!

kmuehlbauer commented Aug 21, 2025

Uh oh!

davidhassell commented Aug 21, 2025

Uh oh!

Uh oh!

valeriupredoi commented Aug 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kmuehlbauer commented Aug 15, 2025 •

edited

Loading

bnlawrence commented Aug 19, 2025 •

edited

Loading

bmaranville commented Aug 19, 2025 •

edited

Loading

kmuehlbauer commented Aug 19, 2025 •

edited

Loading

codecov Bot commented Aug 20, 2025 •

edited

Loading