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

Troublesome type checks in rpnpy #57

Closed
wobagi opened this issue May 9, 2023 · 16 comments
Closed

Troublesome type checks in rpnpy #57

wobagi opened this issue May 9, 2023 · 16 comments

Comments

@wobagi
Copy link

wobagi commented May 9, 2023

The version of rpnpy bundled with fstd2nc has an issue with numpy dtype checks. It worked fine with numpy < 1.20 however since numpy 1.20 the code throws an exception when trying to write fstd files.

TypeError: dtype_numpy2fst: Expecting arg of type numpy.dtype, Got <class 'numpy.dtype[float32]'>

The offending code is located in fstd2nc_deps/rpnpy/librmn/fstd98.py lines 162 and 167. I did not dig deeper into the issue, but my workaround is to do the type checking in a more canonical way and replace line 162

if not (type(npdtype) == _np.dtype or type(npdtype) == type):

with

if not isinstance(npdtype, _np.dtype) or isinstance(npdtype, type)

and line 167

if dtype == npdtype:

with

if isinstance(npdtype, dtype):

however doing so with every fstd2nc install is a bit frustrating. Do you think you can include this fix or another potentially better solution in fstd2nc-deps ? Thank you

@neishm
Copy link
Owner

neishm commented May 9, 2023

This particular issue is fixed in the latest release of rpnpy (2.2.0), which is scheduled have a public release soon (in a few days?). Once that's available I can update the dependencies in this package.

@wobagi
Copy link
Author

wobagi commented May 9, 2023

Awesome. Looking forward to it. Thanks a lot

@neishm
Copy link
Owner

neishm commented May 16, 2023

The new release of rpnpy is still pending, so in the mean time I've uploaded a patched version of the old one (rpnpy version 2.1-rc11). New installs of fstd2nc should get that version automatically. For existing installs, the bundled rpnpy can be upgraded by running pip install --upgrade fstd2nc-deps.

@wobagi
Copy link
Author

wobagi commented May 17, 2023

Thank you.
I tried to upgrade it and got some pretty long list of errors.
Installing a fresh version gave the same result, on python 3.10 and 3.11 (OS X Monterey).
This looks like an issue with gfortran and may take me a while to figure out. If I can find what is missing and make it work I will post here.
Best

@neishm
Copy link
Owner

neishm commented May 17, 2023

Only the Linux and Windows binary wheels were done at the moment, the OS X one would be building from source. From what I recall you would need gfortran installed from homebrew. Unfortunately I don't have an OS X machine to test on, but maybe if you send the errors (maybe as a text file attachment) we could figure out what the issue is. I'll also see if I can resurrect the old OS X build service (been a few years, I don't think Travis-CI is still activated in my github account...)

@neishm
Copy link
Owner

neishm commented May 17, 2023

Ok, I've located one possible problem in the source build. There's some code in the fstd2nc-deps packaging which updates the wheel filename to indicate it works for any Python version. This was necessary for building a manylinux binary wheel, but not needed for standard source builds on the user end. And for recent versions of Python, it no longer likes this type of wheel filename. Could you try installing from the following test version, and see if it still gives you an error?

pip install fstd2nc-deps --extra-index-url https://test.pypi.org/simple/

If that works, I'll rebuild and update the package on pypi.

@wobagi
Copy link
Author

wobagi commented May 17, 2023

It looks like it is related to the fortran compiler. I reinstalled gcc fortran with Homebrew and tried again. I am currently testing on Python 3.10, but I think it is not relevant. I am attaching the log file. One thing is that gfortran compiles it as 2018 dialect and gives some warnings on that. It was not a problem before, though, so I don't think it matters. I will try setting an older dialect explicitly to see. However the first errors on type mismatch do not make much sense to me.

fstd2nc_log.txt

@neishm
Copy link
Owner

neishm commented May 17, 2023

Ok, so there seems to be multiple issues with the source build. My fix was for a different problem. Your gfortran issue appears to be a compatibility break with gcc version 10. Found some discussion at Unidata/netcdf-fortran#212. The workaround is to set some compiler flags:

export FCFLAGS="-w -fallow-argument-mismatch -O2"
export FFLAGS="-w -fallow-argument-mismatch -O2"

If you set those flags, does it get further in the installation?

@neishm
Copy link
Owner

neishm commented May 18, 2023

I was able to reproduce some of the gcc 10 issues in an Ubuntu 22.04 Docker image. I get further in the installation with the following flags set:

export FFLAGS="-fallow-argument-mismatch -fallow-invalid-boz"

However near the end when it's linking librmn together it's complaining about a duplicate symbol for fst_missing.

@wobagi
Copy link
Author

wobagi commented May 18, 2023

I am not quite sure where I should pass those flags? I am not too familiar with the fortran builds inside python modules.
I also tried python-rpn-wheel adding the flags to the Makefile and using make native, but I did not notice any changes.

@neishm
Copy link
Owner

neishm commented May 18, 2023

I think the export statement would work from the command-line (although I don't have an OS X machine to test).

I just added the extra flags to the package and re-uploaded to PyPI. Could you try re-testing again with pip install --upgrade fstd2nc-deps? (no extra flags should be needed on the user end).

@wobagi
Copy link
Author

wobagi commented May 18, 2023

Yeah, I tried exporting the flags in the terminal before but it did not make any difference. I thought that maybe I was supposed to put them in the Makefile.
Anyways, I checked the newest version of fstd2nc_deps (0.20200304.4) and it still throws the same errors. I am attaching the log file.

20230518_201000.txt

@neishm
Copy link
Owner

neishm commented May 18, 2023

Looks like it got a bit further this time, at least. The new error message is

2023-05-18T20:11:21,172     ipsort.c:308:8: error: implicitly declaring library function 'strncmp' with type 'int (const char *, const char *, unsigned long)' [-Werror,-Wimplicit-function-declaration]
2023-05-18T20:11:21,172              if ( strncmp ( list + (*(index + i) - 1)*len,
2023-05-18T20:11:21,172                   ^
2023-05-18T20:11:21,172     ipsort.c:308:8: note: include the header <string.h> or explicitly provide a declaration for 'strncmp'
2023-05-18T20:11:21,172     1 error generated.

Some googling indicates this is because recent macOS versions the default behaviour in gcc is to raise an error on implicit declaration of library functions. The default on other platforms like Linux is to print a warning and continue.
I just added the extra compiler flag -Wno-implicit-function-declaration to the source package to work around this.

Can you retry again and see if it helps?

@wobagi
Copy link
Author

wobagi commented May 18, 2023

Success!!
I briefly tested a couple of functions and it seems to work like a charm.
Thank you so much for the effort to find the solution. Greatly appreciated!

@neishm
Copy link
Owner

neishm commented May 18, 2023

You're welcome! Glad it's finally working!

@neishm
Copy link
Owner

neishm commented May 19, 2023

Closing this issue since it's appears to be resolved. The issue related to updating fstd2nc-deps to rpnpy 2.2.0 has been moved to neishm/python-rpn-wheel#2

@neishm neishm closed this as completed May 19, 2023
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

No branches or pull requests

2 participants