Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Please test the beta release: V0.12.0 #364

Merged
merged 16 commits into from
Feb 15, 2023
Merged

Please test the beta release: V0.12.0 #364

merged 16 commits into from
Feb 15, 2023

Conversation

bastibe
Copy link
Owner

@bastibe bastibe commented Feb 6, 2023

Dear soundfile community,

Find attached a beta release of the next version of soundfile, with

  • libsndfile v1.2.0
  • manylinux binary wheel
  • improved libsndfile discovery

I would be grateful if you could test the wheels on the platform of your choice, and report back if they work, or if there are any issues.

soundfile-0.12.0.wheels.zip

peircej and others added 9 commits January 31, 2023 10:10
Rather than trying to create the location for `_soundfile_data/` manually, simply import it and use its reported `__path__`

The existing method fails if using py2app because:

- starting path is `/lib/python38.zip/soundfile.py`
- desired path is `/lib/python3.8/_soundfile_data`
- the previous method of stepping back from the starting path never reaches this desired path (it tries `/lib/_soundfile_data` and fails)
The `__path__` is a _Namespacepath object and it's a list
of strings not a single string
Adding an empty `__init__.py` to the `_soundfile_data` package means
that the __file__ attribute exists so we can do
```
import _soundfile_data
_path = os.path.dirname(_soundfile_data.__file__)
_full_path = _os.path.join(_path, _packaged_libname)
_snd = _ffi.dlopen(_full_path)
```
apparently, multi-OS tags are no longer allowed.
@bmcfee
Copy link

bmcfee commented Feb 6, 2023

Hrm, I just gave this a spin in a fresh conda environment, and it does not appear to be working. I think it's pulling my libsndfile from the operating system and not the wheel:

libsndfile1/jammy,now 1.0.31-2build1 amd64 [installed]
  Library for reading/writing audio files

libsndfile1-dev/jammy,now 1.0.31-2build1 amd64 [installed]
  Development files for libsndfile; a library for reading/writing audio files
→  which pip
/home/bmcfee/miniconda/envs/sndfile-testing/bin/pip
 🐍 sndfile-testing  ~/sndfiletest 
 →  pip install ./soundfile-0.12.0-py2.py3-none-manylinux_2_31_x86_64.whl 
Processing ./soundfile-0.12.0-py2.py3-none-manylinux_2_31_x86_64.whl
Requirement already satisfied: cffi>=1.0 in /home/bmcfee/miniconda/envs/sndfile-testing/lib/python3.9/site-packages (from soundfile==0.12.0) (1.14.6)
Requirement already satisfied: pycparser in /home/bmcfee/miniconda/envs/sndfile-testing/lib/python3.9/site-packages (from cffi>=1.0->soundfile==0.12.0) (2.21)
Installing collected packages: soundfile
Successfully installed soundfile-0.12.0
 🐍 sndfile-testing  ~/sndfiletest 
 →  cd
 🐍 sndfile-testing  ~ 
 →  python
Python 3.9.0 | packaged by conda-forge | (default, Nov 26 2020, 07:57:39) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import soundfile
>>> soundfile.__libsndfile_version__
'1.0.31'
>>> soundfile.__version__
'0.11.0'
>>> soundfile.__file__
'/home/bmcfee/miniconda/envs/sndfile-testing/lib/python3.9/site-packages/soundfile.py'
>>> 

@bastibe
Copy link
Owner Author

bastibe commented Feb 7, 2023

@bmcfee thank you for testing the library!

It seems that soundfile is picking up your system's libsndfile. If you have libsndfile installed on your system, soundfile will always try that first, before trying to load its own. You should see the soundfile-provided libsndfile if you uninstall/mask the system's libsndfile.

But you did find a bug regardless, I forgot to update the __version__. I updated the wheels with the correct __version__.

@Barabazs
Copy link

Barabazs commented Feb 7, 2023

To test it, I added a line after 149 with _libname = None

_libname = _find_library('sndfile')
if _libname is None:
raise OSError('sndfile library not found')

Doing this, it loads the provided libsndfile and the version is 1.2.0

>>> import soundfile
>>> soundfile.__libsndfile_version__
'1.2.0'
>>> 

I also tested loading an MP3 with librosa.load() and soundfile.read() and this worked both times.

It seems that soundfile is picking up your system's libsndfile. If you have libsndfile installed on your system, soundfile will always try that first, before trying to load its own. You should see the soundfile-provided libsndfile if you uninstall/mask the system's libsndfile.

Imo this should be reversed. Messing with the system installed packages does not seem like a good plan.

@bastibe
Copy link
Owner Author

bastibe commented Feb 7, 2023

@Barabazs thank you for testing the lib!

Imo this should be reversed. Messing with the system installed packages does not seem like a good plan.

I understand and perhaps even agree with that. However, let's discuss this in a separate issue. Please open an issue to collect opinions on the load order.

@bmcfee
Copy link

bmcfee commented Feb 7, 2023

It seems that soundfile is picking up your system's libsndfile. If you have libsndfile installed on your system, soundfile will always try that first, before trying to load its own. You should see the soundfile-provided libsndfile if you uninstall/mask the system's libsndfile.

Right, that's exactly what's happening. It's maybe worth noting that this does not happen with the conda build, likely because conda environments will prioritize the local env's LD_LIBARARY_PATH above the operating system. This seems sane to me, under the principle of going from most specific to least specific search path, but that's just my take.

That said, it's probably a good idea if all distributions of the package were consistent in this behavior (and across platforms - i've only tested linux), just to cut down on the possibility of unpredictable bugs down the road.

Bastian Bechtold added 2 commits February 9, 2023 09:58
Previously, we tried to load the system libsndfile before trying our
own packaged libsndfile, to give users the option to use custom,
potentially newer versions of libsndfile.

However, this situation has reversed in the last few versions of
libsndfile. These introduced some new features and much-needed bug
fixes, and soundfile has been updated to provide feature-complete,
up-to-date versions of libsndfile on most platforms. Linux package
repositories, however, have largely not yet updated to these versions,
making it desirable to use our libsndfile over the system-provided
ones.

Should the system-provided libsndfile be preferred over the bundled
libsndfile, one should install the source packages or source
wheels (or indeed, a system-provided soundfile).
@bastibe
Copy link
Owner Author

bastibe commented Feb 9, 2023

After thinking about the problem some more, I think I have come around to this point of view as well. We should indeed prioritize our packaged libsndfile over the system-provided one, especially on linux, where the system's libsndfile is likely outdated. (It really doesn't matter much on Windows or macOS, since there's unlikely to be a system libsndfile anyway).

As such, I have updated the wheels to first look for the packaged libsndfile, and only fall back to the system libsndfile if the packaged one is unavailable or incompatible.

Please do test this again to make sure I didn't break anything in doing so! (The link to the wheels in the initial post of this PR has been updated)

where the machine was incorrectly identified.
@bmcfee
Copy link

bmcfee commented Feb 9, 2023

I gave this a spin, and hit some snags.

The first problem was my fault - I cloned the repo and switched to this branch without pulling in the submodule for the data files. The build script still ran successfully, but the wheels did not include the .so files and that tripped me up. I didn't see anything about submodules / pull --recurse in the contributing guidelines, so you might want to add a note there.

After fixing the submodule, I rebuilt the packages, and it looks like it should work for most platforms, but the linux build is still missing the .so file (obvious from the file sizes, confirmed by checking with unzip):

 🐍 sndfile-testing   v0.12.0 +  ~/src/python-soundfile 
 →  ls -l dist
total 4220
-rw-rw-r-- 1 bmcfee bmcfee   23681 Feb  9 07:03 soundfile-0.12.0-py2.py3-none-any.whl
-rw-rw-r-- 1 bmcfee bmcfee 1212695 Feb  9 07:03 soundfile-0.12.0-py2.py3-none-macosx_10_9_x86_64.whl
-rw-rw-r-- 1 bmcfee bmcfee 1094038 Feb  9 07:03 soundfile-0.12.0-py2.py3-none-macosx_11_0_arm64.whl
-rw-rw-r-- 1 bmcfee bmcfee   33034 Feb  9 07:03 soundfile-0.12.0-py2.py3-none-manylinux_2_31_x86_64.whl
-rw-rw-r-- 1 bmcfee bmcfee  887094 Feb  9 07:03 soundfile-0.12.0-py2.py3-none-win32.whl
-rw-rw-r-- 1 bmcfee bmcfee 1007969 Feb  9 07:03 soundfile-0.12.0-py2.py3-none-win_amd64.whl
-rw-rw-r-- 1 bmcfee bmcfee   42426 Feb  9 07:03 soundfile-0.12.0.tar.gz

Contrast with the wheel archive @bastibe posted at the beginning of this PR:

 🐍 sndfile-testing  ~/sndfiletest 
 →  ls -l
total 5264
-rw-rw-r-- 1 bmcfee bmcfee 1212764 Feb  6 10:39 soundfile-0.12.0-py2.py3-none-macosx_10_9_x86_64.whl
-rw-rw-r-- 1 bmcfee bmcfee 1075436 Feb  6 10:39 soundfile-0.12.0-py2.py3-none-macosx_11_0_arm64.whl
-rw-rw-r-- 1 bmcfee bmcfee 1174205 Feb  6 10:39 soundfile-0.12.0-py2.py3-none-manylinux_2_31_x86_64.whl
-rw-rw-r-- 1 bmcfee bmcfee  887692 Feb  6 10:39 soundfile-0.12.0-py2.py3-none-win32.whl
-rw-rw-r-- 1 bmcfee bmcfee 1008568 Feb  6 10:39 soundfile-0.12.0-py2.py3-none-win_amd64.whl

Again, the wheel builds without failure, but now it fails to import:

>>> import soundfile
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'soundfile'
>>> import soundfile
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/bmcfee/miniconda/envs/sndfile-testing/lib/python3.9/site-packages/soundfile.py", line 162, in <module>
    _path = _os.path.dirname(_soundfile_data.__file__)
  File "/home/bmcfee/miniconda/envs/sndfile-testing/lib/python3.9/posixpath.py", line 152, in dirname
    p = os.fspath(p)
TypeError: expected str, bytes or os.PathLike object, not NoneType

Looks like the problem is that the _soundfile_data submodule does not actually have the linux_x86_64.so binary, at least not for the version hash used in this branch. Switching the submodule over to the master branch fixes this, and now it works:

>>> import soundfile as sf
>>> sf.__version__
'0.12.0'
>>> sf.__libsndfile_version__
'1.2.0'

Summary: I expect most of my problems here are due to my unfamiliarity with the repo structure and build process for this package, but the end result does appear to work as expected.

@bastibe
Copy link
Owner Author

bastibe commented Feb 9, 2023

Yes, we deliver the binaries in the _soundfile_data submodule, which needs to be updated as well when building the wheels. This should probably be made more explicit in the documentation.

@bmcfee
Copy link

bmcfee commented Feb 9, 2023

Great, LIS it's probably PEBKAC on my side 😁

But it might not hurt to make the build script a little more sensitive to missing object files if they really need to be there.

Bastian Bechtold added 2 commits February 10, 2023 08:09
no longer dies with a TypeError if _soundfile_data is empty, but falls
back to the system library as it should.
@bastibe
Copy link
Owner Author

bastibe commented Feb 10, 2023

I've added some documentation on the wheel build process, and fixed your TypeError.

_explicit_libname = 'libsndfile.dylib'
elif _sys.platform == 'win32':
_explicit_libname = 'libsndfile.dll'
elif _sys.plaform == 'linux':

Choose a reason for hiding this comment

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

Here it is

Copy link
Owner Author

@bastibe bastibe Feb 15, 2023

Choose a reason for hiding this comment

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

Fixed in #370, and published as v0.12.1

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.

5 participants