HDFGroup / hdf-compass

Python-based viewer for HDF5 on other file formats
Other
131 stars 26 forks source link

ADIOS plugin #173

Closed michaelsippel closed 8 years ago

michaelsippel commented 8 years ago

This PR adds the plug-in implementation to browse ADIOS-files with HDF-Compass. Additional dependencies for using this plug-in are: ADIOS (1.9.0+) with numpy wrapper (1.9.1b18+).

Note: currently this implementation was only tested with python2 since the unit-tests from HDF-Compass don't work with python3. When these errors are fixed, I will check the support for the ADIOS-plugin too.

CC @ax3l

ax3l commented 8 years ago

@michaelsippel can you please fix the left over travis errors with the example file? See: https://travis-ci.org/ComputationalRadiationPhysics/hdf-compass/jobs/143174332#L2627

michaelsippel commented 8 years ago

@ax3l My guess is the adios-numpy wrappers wich are installed by pip are outdated. We need to build them in travis from source.

ax3l commented 8 years ago

Looks like it, can do. (update: done) Or I ask them if they can release a recent version on pypi. (update: https://github.com/ornladios/ADIOS/issues/75; update setup.py requirement after that, too!)

ax3l commented 8 years ago

@jreadey @giumas are you interested in giving us a review (and merge)? :)

jreadey commented 8 years ago

@ax3l I emailed a @michaelsippel a copy of our contributors agreement, but I haven't heard back yet. Actually it would be best for you to fill it out as well. Once we have these on file, we'll be able to merge this this PR and any future ones.

Also, can you point me to some sample files in the ADIOS format?

ax3l commented 8 years ago

@jreadey thank you, we just sent you the documents back.

We have committed a small adios .bp example file with this PR. They come in two flavours: an "all-in-one" file .bp and a "distributed" format consisting of a .bp file with meta information and a directory .bp.dir with sub-files. API-wise both are transparent to the user.

ax3l commented 8 years ago

@giumas it seems travis-ci is not enabled by default for the repo. you might want to enable it? (all tests are passing.)

jreadey commented 8 years ago

I've enabled trade-cl for compass. @ax3l do you want to push something to the branch and verify that that triggers a test run?

ax3l commented 8 years ago

it seems adios is not included by default by the packager yet (pyinstaller). do you have any hints what to add? Locally, I just shuffled adios into the "hidden imports" to force it to be included, but there might be a clean way (e.g., for a follow-up PR to build the travis aws artefacts correctly). (did not work)

ax3l commented 8 years ago

@jreadey will do!

jreadey commented 8 years ago

What was the change you made locally?

I'll take a look later today.

ax3l commented 8 years ago

I tried hacking

diff --git a/HDFCompass.1file.spec b/HDFCompass.1file.spec
index d20fd20..f5e1fc6 100644
--- a/HDFCompass.1file.spec
+++ b/HDFCompass.1file.spec
@@ -50,7 +50,8 @@ def collect_pkg_data(package, include_py_files=False, subdir=None):
     return data_toc

 pkg_data_hdf_compass = collect_pkg_data('hdf_compass')
-pkg_data_bag = collect_pkg_data('hydroffice.bag')
+pkg_data_adios = collect_pkg_data('adios')
+#pkg_data_bag = collect_pkg_data('hydroffice.bag')
 cartopy_aux = []
 try:  # for GeoArray we use cartopy that can be challenging to freeze on OSX to dependencies (i.e. geos)
     import cartopy.crs as ccrs
@@ -71,7 +72,7 @@ app_name = 'HDFCompass_' + version
 a = Analysis(['HDFCompass.py'],
              pathex=[],
              hiddenimports=['scipy.linalg.cython_blas', 'scipy.linalg.cython_lapack',
-               'scipy.linalg', 'scipy.integrate'],  # for cartopy
+               'scipy.linalg', 'scipy.integrate', 'adios'],  # for cartopy
              excludes=["PySide"],  # exclude libraries from being bundled (in case that are installed)
              hookspath=None,
              runtime_hooks=None)
@@ -83,7 +84,8 @@ exe = EXE(pyz,
           a.zipfiles,
           a.datas,
           pkg_data_hdf_compass,
-          pkg_data_bag,
+          pkg_data_adios,
+#          pkg_data_bag,
           cartopy_aux,
           name=app_name,
           debug=True,

but that did not do the trick as I just found out.

ax3l commented 8 years ago

ok we can do packing in a follow-up PR. I think it's both necessary to fix the a = Analysis(['HDFCompass.py'], line to include the hdf_compass/compass_viewer/viewer.py that includes all models and additionally, there seems to be a smaller upstream issue (_hl helpers) within adios that I can report/fix first before packing.

giumas commented 8 years ago

I tried hacking

This is not the way that I would follow to freeze HDF Compass with the adios plugin.

Is the adios package in pure-Python? If not, the issue is most likely that there is not currently a hook for this package in PyInstaller: https://github.com/pyinstaller/pyinstaller/tree/develop/PyInstaller/hooks

If you are not familiar, a PyInstaller's hook is a file used to define which additional files should be included by PyInstaller (compiled binaries, xml files, images, etc) in order to use a package in a frozen application.

This has nothing to do directly with HDF Compass, since any application using adios would have the same issue when freezing using PyInstaller. Try to make it working with a minimal file that just import adios. The PyInstaller developers are very responsive in adding new hooks if you make a PR for it.

giumas commented 8 years ago

ok we can do packing in a follow-up PR.

Since the freezing step is very relevant for HDF Compass, I would suggest to first try to fix the issue.

I think it's both necessary to fix the a = Analysis(['HDFCompass.py'], line to include the viewer.py that includes all models and additionally

I doubt that this is required.

there seems to be a smaller upstream issue (_hl helpers) within adios that I can report/fix first before packing

Can you post more info on this issue?

ax3l commented 8 years ago

@giumas thanks for your suggestions.

The adios module is Cython afaik, here is its source: ornladios/ADIOS

I solved it now by this simple diff:

diff --git a/HDFCompass.1file.spec b/HDFCompass.1file.spec
index d20fd20..1d5967c 100644
--- a/HDFCompass.1file.spec
+++ b/HDFCompass.1file.spec
@@ -71,7 +71,7 @@ app_name = 'HDFCompass_' + version
 a = Analysis(['HDFCompass.py'],
              pathex=[],
              hiddenimports=['scipy.linalg.cython_blas', 'scipy.linalg.cython_lapack',
-               'scipy.linalg', 'scipy.integrate'],  # for cartopy
+               'scipy.linalg', 'scipy.integrate', 'adios._hl.selections'],  # for cartopy
              excludes=["PySide"],  # exclude libraries from being bundled (in case that are installed)
              hookspath=None,
              runtime_hooks=None)

I probably just screwed up my env in the first tries.

The patched line is a hidden include in adios that is otherwise not found at runtime. I think I can report that to be fixed upstream instead. depends on your preferences. (update: tried fixing it with renaming without the _ prefix but did not resolve the need for the hint. I will write the pyinstaller hook for it.)

system info: I am packing on Debian 8.5 "jessie" via pyinstaller --clean -y HDFCompass.1file.spec.

ax3l commented 8 years ago

ok the PyInstaller hook PR is open.

Can you try if the OSX (py2app) packing and Windows packing is working? (the ADIOS c-lib and numpy wrapper will likely only be available on linux/osx)

jreadey commented 8 years ago

Is ADIOS supported on Windows? I was looking at the INSTALL file on github and it mentions Linux and Mac OS X but not our favorite Redmond software vendor.

ax3l commented 8 years ago

afaik not, it will be a Linux/OSX plugin only. I just wanted to make sure I am not breaking the Windows packaging due to side effects (likely still ok). Maybe you want to enable the appveyor tests on this repo, too?

ax3l commented 8 years ago

@giumas @jreadey since all tests pass and packaging only depends on the upcoming pyinstaller update (until then: simply not included but other plugins still working) this PR is actually feature complete.

giumas commented 8 years ago

afaik not, it will be a Linux/OSX plugin only.

The README.rst should be modified to cite it, as well as added the adios dependency.

I just wanted to make sure I am not breaking the Windows packaging due to side effects (likely still ok). Maybe you want to enable the appveyor tests on this repo, too?

I tested on Windows, and the application works showing the message ADIOS plugin: NOT loaded as expected.

As a general comment, I am a bit surprised that being a Linux/OSX-only plugin was not specified from the beginning. However, I ran through the code and it looks quite consistent. I leave to @jreadey the burden to accept a plugin that is not cross-platform (all the other plugins are).

Incidentally, @michaelsippel and @ax3l : can you share a frozen OSX HDF Compass application so that I can test if it works on a clean machine?

jreadey commented 8 years ago

I think it's ok to have plugins that are platform specific as long as it doesn't introduce problems for supporting the other problems.

@giumas - where you able to build the ADIOS package on linux or mac? I see it requires MPI, which seems a little odd for a desktop app!

giumas commented 8 years ago

@jreadey I don't have currently access to my work OSX machine, but I have clean OSX VMs. This is why I asked about the frozen app.

jreadey commented 8 years ago

@ax3l - do I actually need MPI to build ADIOS? E.g. I don't have mpicc on my desktop.

What MPI-sh things will the ADIOS Compass plugin do?

ax3l commented 8 years ago

The README.rst should be modified to cite it, as well as added the adios dependency.

will do, thanks!

Windows support

Since ADIOS is a research, HPC-only transport and I/O library by Oak Ridge National Lab this has not been in focus. I am glad we can still use it on the other two platforms, thanks! In the future, the maintainers of ADIOS are actually looking forward to try supporting Windows as well, the progress of the discussion can be followed at https://github.com/ornladios/ADIOS/issues/77 .

Incidentally, @michaelsippel and @ax3l : can you share a frozen OSX HDF Compass application so that I can test if it works on a clean machine?

yes, I already did build at Linux 64bit ELF a few days ago. You can download it from:

MPI dependency

This is an excellent question and let me shine some light on that. Of course, we do not need any MPI functionality at all and the final plugin also has no MPI dependencies. The only reason we need MPI right now is issue https://github.com/ornladios/ADIOS/issues/74 which hopefully going to be fixed in the mid term, thanks to the maintainer and core developer of ADIOS @pnorbert.

The c-library ADIOS creates two libraries: libadios and libadios_nompi. The numpy binding then again consists of two independent builds: adios_mpi and adios (no MPI). We of course only depend on libadios_nompi and adios. The only problem is currently, that ADIOS' build scripts can only build the two targets at once. I hope that is no problem for now and I can reduce the travis build scripts as soon as https://github.com/ornladios/ADIOS/issues/74 is fixed (I also linked a docker script there for building it).

giumas commented 8 years ago

yes, I already did build at Linux 64bit ELF a few days ago.

@ax3l : since I asked for a Mac OSX frozen app, what do you mean with that?

On the same topic, if you were successful in freezing the app on OSX:

We need this information since they will be useful when it will come the time of a new release. This also helps @jreadey to track the minimum supported/tested OS. In particular, this is very relevant for Mac OSX: https://pythonhosted.org/PyInstaller/usage.html#making-mac-os-x-apps-forward-compatible

In other words, we ideally want to achieve a statement like this: "A working frozen HDF Compass app with the adios plugin has been built and tested on:

If for any reason we are not currently able to freeze the app on OSX we might start by making the plugin Linux-only.

@ghisvail : what are your Debianic thoughts about this optional plugin?

ax3l commented 8 years ago

Incidentally, @michaelsippel and @ax3l : can you share a frozen OSX HDF Compass application so that I can test if it works on a clean machine?

yes, I already did build at Linux 64bit ELF a few days ago.

@ax3l : since I asked for a Mac OSX frozen app, what do you mean with that?

@giumas sorry for the misunderstanding, you mentioned your OSX VMs in a later post I oversaw the "OSX" before "HDF". I had no hands on OSX yet, that's why I am asking if someone can try the build there. As I said, travis-builds on OSX 10.9 work as you can see here - but these scripts do not try packing it.

I like the way to list and test the supported versions. So far I did the HDFCompass tests on:

Building adios (c lib and numpy wrapper) worked for me on any *nix I came around:

Any OSX testing is super-welcome, I have no VMs around for it but know the ADIOS developers use OSX machines and @yyalli and @suchyta1 are currently testing it out (feel free to join with your test results!).

Regarding the pyinstaller text you linked: aren't you using py2app for OSX packing? (Your readme states so.)

ax3l commented 8 years ago

Updated the following aspects:

jreadey commented 8 years ago

I've merged to the develop branch!

ax3l commented 8 years ago

thank you! :fireworks: