Skip to content

Enhance setup.py#1026

Merged
jswhit merged 6 commits into
Unidata:masterfrom
sroet:fix_setup
Jul 21, 2020
Merged

Enhance setup.py#1026
jswhit merged 6 commits into
Unidata:masterfrom
sroet:fix_setup

Conversation

@sroet
Copy link
Copy Markdown
Contributor

@sroet sroet commented Jul 20, 2020

Since #1024 this package uses tobytes(), however this function only exists for numpy>=1.9 (numpy release note reference).

This PR bumps the numpy version.

While trying to fix this, I ran into the issue that a pip install -e . would not check my active conda environment for the presence of HDF5 headers, so I also added that to the setup.py.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jul 20, 2020

CLA assistant check
All committers have signed the CLA.

Comment thread setup.py Outdated

dirstosearch = [os.path.expanduser('~'), '/usr/local', '/sw', '/opt',
'/opt/local', '/usr']
dirstosearch = [os.getenv("CONDA_PREFIX", ""), os.path.expanduser('~'),
Copy link
Copy Markdown
Contributor Author

@sroet sroet Jul 20, 2020

Choose a reason for hiding this comment

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

I choose to look for the environment first, because pip will (normally) try to install into this environment as well.

Not looking there first might lead to strange errors if the HDF5 headers in that environment are not the one it is installing with.

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.

Any non-standard directory for dependencies should be specified with the env vars. Also, if you are using conda you should probably get the pre-built binaries from conda and not installing from source with pip.

Copy link
Copy Markdown
Contributor Author

@sroet sroet Jul 20, 2020

Choose a reason for hiding this comment

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

Also, if you are using conda you should probably get the pre-built binaries from conda and not installing from source with pip.

This is true for most/all users. But if you want to do some development/testing (such as with this PR), you have to use pip install -e .. If you use conda as your main way for different python environments, there is not an easy way to do that with the current setup.py.

This list already contains less-standard directories, such as /sw, and if CONDA_PREFIX exist there is a big chance that this package will actually be installed with code under that prefix as well (as both pip and python will run under that prefix)

Also, having hdf5 installed in a conda environment will actually override (on runtime) any linking of HDF via other directories or HDF5_DIR on buildtime. Because cython is also installed under CONDA_PREFIX and loads libraries from there as well. This can lead to very unexpected (or undefined) behavior.

Example: Linking with HDF5_DIR to 1.8.16 but netcdf4 uses 1.10.6 anyway. (click me)

# start in a the clone of this repo (master branch)
# create a conda env with hdf 1.8.16, this can be skipped with a preinstalled version in one of the standard directories
conda create -n hdf hdf5==1.8.16 -y
# Now create an environment for testing and activate
conda create -n test python=3 -y
conda activate test

# Install cython and netcdf4 dependencies
conda install -y cython
conda install -y --only-deps netcdf4

# Show that it now installed hdf=1.10.6
conda list hdf5

# Now we are going to install netcdf4, but explicitly point HDF5_DIR to the 1.8.16 conda environment (this can be done without HDF5_DIR if hdf5 is installed in one of the standard dirs)
HDF5_DIR=$CONDA_PREFIX/../hdf pip install -e .

# Let's see what is picked up in the test
cd test
python run_all.py

expected output:

netcdf4-python version: 1.5.4
HDF5 lib version:       1.8.16
netcdf lib version:     4.7.4
numpy version           1.19.0
cython version          0.29.21

Actual output:

netcdf4-python version: 1.5.4
HDF5 lib version:       1.10.6
netcdf lib version:     4.7.4
numpy version           1.19.0
cython version          0.29.21

As you can see in that example it grabs the HDF5 from the current conda environment anyway, so why not link against it on build time?

Even more broken behavior if hdf5 is not present in the env during build-time and run-time (click me)

Starting from the end of the previous example(conda environment test is active, cwd is netcdf4-python/test)

# Go back to main directory
cd ../

# break environment by force removing hdf5
conda remove hdf5 --force -y

# install netcdf4 again, HDF5_DIR is not needed if hdf5 is already installed in one of the standard dirs
HDF5_DIR=$CONDA_PREFIX/../hdf pip install -e .

# Let's see if we can run the test suite
cd test
python run_all.py

This fails with an import error:

Traceback (most recent call last):
  File "run_all.py", line 2, in <module>
    from netCDF4 import getlibversion,__hdf5libversion__,__netcdf4libversion__,__version__
  File "/home/sroet/github_files/netcdf4-python/netCDF4/__init__.py", line 3, in <module>
    from ._netCDF4 import *
ImportError: libhdf5_hl.so.100: cannot open shared object file: No such file or directory

If hdf5 is installed to environment after this and netcdf4 is not rebuild, hdf5 dumps its core (click me)

Continue the previous example (broken by force removing hdf5, cwd=netcdf4-python/test) by installing hdf5 but not rebuilding netcdf4.

conda install -y hdf5
python run_all.py

Now HDF5 dumps it's core:

not running tst_unicode.py ...

netcdf4-python version: 1.5.4
HDF5 lib version:       1.8.16
netcdf lib version:     4.7.4
numpy version           1.19.0
cython version          0.29.21
.Warning! ***HDF5 library version mismatched error***
The HDF5 header files used to compile this application do not match
the version used by the HDF5 library to which this application is linked.
Data corruption or segmentation faults may occur if the application continues.
This can happen when an application was compiled by one version of HDF5 but
linked with a different version of static or shared HDF5 library.
You should recompile the application or check your shared library related
settings such as 'LD_LIBRARY_PATH'.
You can, at your own risk, disable this warning by setting the environment
variable 'HDF5_DISABLE_VERSION_CHECK' to a value of '1'.
Setting it to 2 or higher will suppress the warning messages totally.
Headers are 1.10.6, library is 1.8.16
	    SUMMARY OF THE HDF5 CONFIGURATION
	    =================================

General Information:
-------------------
		   HDF5 Version: 1.8.16
		  Configured on: Sun Aug  7 21:39:57 UTC 2016
		  Configured by: root@08a75980133e
		 Configure mode: production
		    Host system: x86_64-unknown-linux-gnu
	      Uname information: Linux 08a75980133e 3.13.0-91-generic #138-Ubuntu SMP Fri Jun 24 17:00:34 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
		       Byte sex: little-endian
		      Libraries: static, shared
	     Installation point: /home/sander/miniconda3/envs/hdf

Compiling Options:
------------------
               Compilation Mode: production
                     C Compiler: /opt/rh/devtoolset-2/root/usr/bin/gcc ( gcc (GCC) 4.8.2 20140120 )
                         CFLAGS: 
                      H5_CFLAGS: -std=c99 -pedantic -Wall -Wextra -Wundef -Wshadow -Wpointer-arith -Wbad-function-cast -Wcast-qual -Wcast-align -Wwrite-strings -Wconversion -Waggregate-return -Wstrict-prototypes -Wmissing-prototypes -Wmissing-declarations -Wredundant-decls -Wnested-externs -Winline -Wfloat-equal -Wmissing-format-attribute -Wmissing-noreturn -Wpacked -Wdisabled-optimization -Wformat=2 -Wunreachable-code -Wendif-labels -Wdeclaration-after-statement -Wold-style-definition -Winvalid-pch -Wvariadic-macros -Winit-self -Wmissing-include-dirs -Wswitch-default -Wswitch-enum -Wunused-macros -Wunsafe-loop-optimizations -Wc++-compat -Wstrict-overflow -Wlogical-op -Wlarger-than=2048 -Wvla -Wsync-nand -Wframe-larger-than=16384 -Wpacked-bitfield-compat -Wstrict-overflow=5 -Wjump-misses-init -Wdouble-promotion -Wsuggest-attribute=const -Wtrampolines -Wstack-usage=8192 -Wvector-operation-performance -Wsuggest-attribute=pure -Wsuggest-attribute=noreturn -Wsuggest-attribute=format -O3
                      AM_CFLAGS: 
                       CPPFLAGS: 
                    H5_CPPFLAGS: -D_GNU_SOURCE -D_POSIX_C_SOURCE=200112L   -DNDEBUG -UH5_DEBUG_API
                    AM_CPPFLAGS: -D_LARGEFILE64_SOURCE -D_LARGEFILE_SOURCE  -I/home/sander/miniconda3/envs/hdf/include
               Shared C Library: yes
               Static C Library: yes
  Statically Linked Executables: no
                        LDFLAGS: 
                     H5_LDFLAGS: 
                     AM_LDFLAGS:  -L/home/sander/miniconda3/envs/hdf/lib
 	 	Extra libraries: -lrt -lz -ldl -lm 
 		       Archiver: ar
 		 	 Ranlib: ranlib
 	      Debugged Packages: 
		    API Tracing: no

Languages:
----------
                        Fortran: no

                            C++: yes
                   C++ Compiler: /opt/rh/devtoolset-2/root/usr/bin/g++ ( g++ (GCC) 4.8.2 20140120 )
                      C++ Flags: 
                   H5 C++ Flags:  
                   AM C++ Flags: 
             Shared C++ Library: yes
             Static C++ Library: yes

Features:
---------
                  Parallel HDF5: no
             High Level library: yes
                   Threadsafety: no
            Default API Mapping: v18
 With Deprecated Public Symbols: yes
         I/O filters (external): deflate(zlib)
                            MPE: no
                     Direct VFD: no
                        dmalloc: no
Clear file buffers before write: yes
           Using memory checker: no
         Function Stack Tracing: no
      Strict File Format Checks: no
   Optimization Instrumentation: no
Bye...
Aborted (core dumped)

If HDF5 is present in one of the standard dirs, all three of these examples are possible. This change catches at least the most common one (present in both the env and somewhere else during build-time), and fixes it.

I should make an issue for situation 2 (not present in the env during build and runtime) and 3 (only present during runtime), but solving those situations is less trivial.

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.

As someone who uses conda for development I disagree. Also /sw used to be pretty standard some years ago. TL;DR I don't have a horse on this race and I don't care if this change is in or out but I don't recommend adding extra code when the directory env var works just fine as long as you don't mix environments like that. (Which you should not do b/c of the activation scripts!)

Copy link
Copy Markdown
Contributor Author

@sroet sroet Jul 21, 2020

Choose a reason for hiding this comment

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

No problem, thanks for the review. I reverted this change and will make an issue to discuss this

Comment thread setup.py Outdated

dirstosearch = [os.path.expanduser('~'), '/usr/local', '/sw', '/opt',
'/opt/local', '/usr']
dirstosearch = [os.getenv("CONDA_PREFIX", ""), os.path.expanduser('~'),
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.

Any non-standard directory for dependencies should be specified with the env vars. Also, if you are using conda you should probably get the pre-built binaries from conda and not installing from source with pip.

Comment thread setup.py
Copy link
Copy Markdown
Collaborator

@ocefpaf ocefpaf left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for reducing the scope and separating the directory discussion. This fixes a bug and my own builds are affected by this. It would be nice to get this merged and a minor release out.

@jswhit jswhit merged commit 2762a5c into Unidata:master Jul 21, 2020
@sroet sroet deleted the fix_setup branch July 22, 2020 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants