F2I-Consulting / fesapi

DevKit for ENERGISTICS™ data standards (mainly RESQML™), multi-languages (C++, Java, C#, Python)
Apache License 2.0
32 stars 24 forks source link

Support for HDF5 >1.12.1 in FESAPI v1.* #323

Closed corppeon closed 9 months ago

corppeon commented 11 months ago

It would be great if FESAPI could support the latest versions of HDF5 - I haven't been able to get it to work with >v1.12.1.

At the time of writing this, the latest HDF5 version is v1.14.1-2. The HDF5 release notes indicate multiple bug, memory leak, and buffer overrun fixes, which it would be good to take advantage of.

philippeVerney commented 10 months ago

Hi @corppeon

Could you please provide more information? Environment, error message, ... I frequently use a FESAPI version 2.7.1.0 built on CentOS 7 linked to HDF5 1.14.0 and I don't have any issue at all. I just rebuilt it again from scratch just to check and still no issue.

corppeon commented 10 months ago

Thanks Philippe,

FESAPI v1.2.1. HDF5 1.14.2, Centos7.9, GCC9.4. I'm seeing compile errors, e.g.

/fesapi/src/resqml2/GridConnectionSetRepresentation.cpp: In member function 'void resqml2::GridConnectionSetRepresentation::setCellIndexPairs(uint64_t, uint64_t, uint64_t, common::AbstractHdfProxy, short unsigned int, short unsigned int)': /fesapi/src/resqml2/GridConnectionSetRepresentation.cpp:108: error: invalid conversion from 'hsize_t' {aka 'long unsigned int'} to 'const long long unsigned int' [-fpermissive] 108 proxy->writeArrayNd(uuid, "CellIndexPairs", H5T_NATIVE_ULLONG, cellIndexPair, numValues, 2);
In file included from /fesapi/src/resqml2/GridConnectionSetRepresentation.cpp:27: /fesapi/src/resqml2/../common/AbstractHdfProxy.h:230: note: initializing argument 5 of 'virtual void common::AbstractHdfProxy::writeArrayNd(const string&, const string&, hdf5_hid_t, const void, const long long unsigned int, unsigned int)' 230 const unsigned long long * numValuesInEachDimension,
/fesapi/src/resqml2/GridConnectionSetRepresentation.cpp:110: error: invalid conversion from 'hsize_t' {aka 'long unsigned int'} to 'const long long unsigned int*' [-fpermissive] 110 proxy->writeArrayNd(uuid, "GridIndexPairs", H5T_NATIVE_USHORT, gridIndexPair, numValues, 2);

In file included from /fesapi/src/resqml2/GridConnectionSetRepresentation.cpp:27: /fesapi/src/resqml2/../common/AbstractHdfProxy.h:230: note: initializing argument 5 of 'virtual void common::AbstractHdfProxy::writeArrayNd(const string&, const string&, hdf5_hid_t, const void, const long long unsigned int, unsigned int)' 230 | const unsigned long long * numValuesInEachDimension,

Regards Andy

philippeVerney commented 10 months ago

Hi @corppeon

HDF5 v1.10+ support is one of the improvement of v2.* of FESAPI : https://github.com/F2I-Consulting/fesapi/releases/tag/v2.0.0.0 (look at "Build" section)

I strongly suggest you to upgrade your FESAPI version.. I don't see any short term possibility for me to work on such an improvement on v1.*

corppeon commented 10 months ago

Hi Philippe,

I seem to be having trouble with detecting HDF5. Compiling 2.8.0.0. It seems to find 1.14.2 but thinks it needs 1.8.18 or greater?

cmake .. -DCMAKE_BUILD_TYPE=Release -DHDF5_ROOT=/hdf5/build/package/HDF_Group/HDF5/1.14.2 -DMINIZIP_INCLUDE_DIR=/minizip -DMINIZIP_LIBRARY_RELEASE=/minizip/build -- The C compiler identification is GNU 9.4.0 -- The CXX compiler identification is GNU 9.4.0 -- Detecting C compiler ABI info -- Detecting C compiler ABI info - done -- Check for working C compiler: /usr/local/gcc-9/bin/gcc - skipped -- Detecting C compile features -- Detecting C compile features - done -- Detecting CXX compiler ABI info -- Detecting CXX compiler ABI info - done -- Check for working CXX compiler: /usr/local/gcc-9/bin/c++ - skipped -- Detecting CXX compile features -- Detecting CXX compile features - done -- Unable to determine HDF5 C flags from HDF5 wrapper. CMake Error at /usr/local/share/cmake-3.25/Modules/FindPackageHandleStandardArgs.cmake:230 (message): Could NOT find HDF5 (missing: HDF5_LIBRARIES) (found suitable version "1.14.2", minimum required is "1.8.18") Call Stack (most recent call first): /usr/local/share/cmake-3.25/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE) /usr/local/share/cmake-3.25/Modules/FindHDF5.cmake:1007 (find_package_handle_standard_args) CMakeLists.txt:57 (find_package)

Regards Andy

philippeVerney commented 10 months ago

Hi Andy,

Now HDF5 dependency is managed through official cmake findHDF5. The FESAPI HDF5 dependency now only relies on this cmake module. Looking at your message, it seems that this cmake findHDF5 module cannot set/find HDF5_LIBRARIES cmake variable although it find enough information to understand you have a 1.14.2 version. I don't know why as I don't know your environment.

A first step could be to verify if you can generate FESAPI makefile with the official binary of HDF1.14.2 by doing similarly as in our CI : https://github.com/F2I-Consulting/fesapi/blob/d51f1dc6a9f189a52ade718e8fee0e618faacf97/.github/workflows/github-actions.yml#L110C3-L110C3

Then if it works, you'll have to better understand what is different in your own build compared to the official build. Maybe you only have static libraries? Maybe you generate the install in some exotic paths? Maybe....

FESAPI only assumes that your HDF5 install can be found and procesed by the cmake findHDF5 module, nothing else.

philippeVerney commented 10 months ago

My link in CI above actually shows a build of 1.14.0, not a usage of the official binary. You can test by building HDF5 1.14.2 the same way as in our CI.

philippeVerney commented 10 months ago

If you REALLY have a too exotic HDF5 install, you can force another kind of dependency to HDF5 by setting the CMAKE VARIABLE WITH_LOCAL_HDF5 to ON and set HDF5_INCLUDE_DIRS and HDF5_LIBRARIES according to your own install.

This is clearly not recommended since it is a sort of hack but sometimes (really exotic HDF5 install) it may be necessary.

corppeon commented 9 months ago

Hi,

I found the issue - I wasn't publishing the cmake subdir for the HDF5 build - this is now needed by the FindHDF5 function.

Thanks Andy

philippeVerney commented 9 months ago

Glad to read that! If no more issue with HDF5 build, please close the issue when you have time.

corppeon commented 9 months ago

Hi again,

I'm struggling to move our code, written against v1.2.1 of FESAPI to v2.8. Lots of classes and method names have changed. Is there any documentation/help on how to migrate v1 to v2?

Thanks Andy

philippeVerney commented 9 months ago

Nothing as you might expect AFAIK. Adopters generally follow releases more frequently than porting from 1.2 to 2.8 and rely :

It is known to be an important shift from v1. to v2.0. Furthermore, some minor versions of v2. have modified the code even more.

FYI, most of adopters port to v2.7+ for ETP (OSDU RDDMS) support,not really for HDF5 AFAIK.

Sorry about that, I would be glad that the FESAPI community provides such contribution. You can use the forum for some migration questions and hope for answers from the community. It could help some other people with the same kind of problems.

corppeon commented 9 months ago

Thanks, so the v2.0 release notes should be a place for me to look?

philippeVerney commented 9 months ago

Yes you can have hints but I think nothing as you expect for such a migration.

However, if you would port minor version by minor version, the release notes would surely be a big help in coordination with the updates in the example project. This is what I observed from adopters who do not outsource their FESAPI code. But the shift from 1.* to v2.0 is clearly an important one.

corppeon commented 9 months ago

Hi,

I've modified my reader code from v1.2.1 to use v2.8, and it compiles and runs most of my test grids (written with v1.2.1).

I'm seeing a problem with one - I'm walking over the DiscreteProperty's in the grid PropertySet, and calling getIntValuesOfPatch() on each of them. This is failing when the property is "ACTNUM", I get a logic_error exception - "Reading integer values from a non integer array is not supported."

Digging into the FESAPI source with the debugger, this maps to "PathInHdfFile="/RESQML/cdf2af29-1da0-470b-a2cc-156077425c35/cellGeometryIsDefined". This appears to have the datatype "gsoap_resqml2_0_1::resqml20__BooleanHdf5Array" which doesn't seem to be supported by getIntValuesOfPatch() ?

Any suggestions? From what I can see, we used to handle this via a no-op in our hdfproxy implementation.

philippeVerney commented 9 months ago

Hi Andy,

First of all, do no hesitate to start a new issue when you actually start a different topic than the title of the issue please. It would help others to better search if they face similar issue.

Short answer: I can add the support of Discrete Property containing Boolean array in next version 2.9.0.0. The standard looks quite flexible on this topic i.e. it does not forbid anything.

Longer answer (which could even be longer) : The general idea is that it would be better to stick to DiscreteProperty only containing integer arrays if you can have the hand on the export process which generated the file. Using a boolean array is "OK" I guess but it does not give you anything.... Even if cellGeometryIsDefined and ACTNUM are different concept, they are related and can be sometimes (even if rarely afaik) equal. In this latter case, it makes sens to share the HDF5 dataset to save memory. Having said that, reusing the cellGeometryIsDefined HDF5 dataset to save memory does not imply to put a gsoap_resqml2_0_1::resqml20__BooleanHdf5Array in a property. It should still better uses a resqml20::IntegerHdf5Array which would point to the same (boolean) HDF5 dataset. Indeed, it does not exist HDF5 boolean dataset (because there is no boolean in HDF5) so in RESQML you are very free to interpret the same HDF5 dataset as an integer one or a boolean one. Interpreting it as a boolean one for cellGeometryIsDefined and as an integer for ACTNUM would make the RESQML file much cleaner imho.

corppeon commented 9 months ago

Yes, that would be good if we could add Boolean support to DiscreteProperty - my models from one writer use booleans, others from a different writer use ints for the same ACTNUM property.

I will close this issue as it has diverged considerably ;)