Skip to content

Use empty blas_ldflags if no cxx is configured #506

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

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

lucianopaz
Copy link
Member

Closes #504

Major / Breaking Changes

  • ...

New features

  • ...

Bugfixes

  • Default blas_ldflags are empty if no compiler is configured

Documentation

  • ...

Maintenance

  • ...

@ricardoV94
Copy link
Member

@lucianopaz do you think it's worth it to add a test?

@lucianopaz
Copy link
Member Author

In all honesty, I don’t know how to test this properly. I know that I can mock the cxx config but I’m not sure if that we’ll have us completely covered. @twiecki, could you try out this branch when there is no compiler and see if it works?

@ricardoV94
Copy link
Member

ricardoV94 commented Nov 17, 2023

In all honesty, I don’t know how to test this properly. I know that I can mock the cxx config but I’m not sure if that we’ll have us completely covered. @twiecki, could you try out this branch when there is no compiler and see if it works?

Why mock? Can't you use the context manager with pytensor.config.change_flags(cxx=""): ....

Edit: Or this fails more internally? If so, ignore me

@ricardoV94 ricardoV94 added bug Something isn't working C-backend labels Nov 17, 2023
@lucianopaz
Copy link
Member Author

In all honesty, I don’t know how to test this properly. I know that I can mock the cxx config but I’m not sure if that we’ll have us completely covered. @twiecki, could you try out this branch when there is no compiler and see if it works?

Why mock? Can't you use the context manager with pytensor.config.change_flags(cxx=""): ....

Edit: Or this fails more internally? If so, ignore me

Yes you're right, I can use the context manager instead. I realized now that I didn't cover the potential for failures in this part of the code. If cxx -print-search-dirs fails to run for whatever reason, it can mean many things:

  1. The cxx command isn't in the path and we wont be able to compile C extensions
  2. The cxx command doesn't have a print-search-dirs argument (gcc and clang do have it, but I don't know if this is a standard C compiler thing)
  3. Something deeper went wrong and we wont be able to compile.

I think that I'll add some guardrails for that case in this PR and write a test for it too.

@lucianopaz
Copy link
Member Author

@ricardoV94, @twiecki, can I bother you to try out this PR and see if it breaks your installations that used:

  • No c compiler
  • Blas on Linux
  • Accelerate on Mac
  • MKL on Intel

@ricardoV94
Copy link
Member

Mine doesn't seem to have changed:

Main:

python -c "import pytensor; print(pytensor.config.blas__ldflags)"
-L/home/ricardo/miniconda3/envs/pytensor/lib -llapack -lblas -lcblas -lm -Wl,-rpath,/home/ricardo/miniconda3/envs/pytensor/lib
PYTENSOR_FLAGS='cxx=""' python -c "import pytensor; print(pytensor.config.blas__ldflags)"
# IndexError: list index out of range

PR:

python -c "import pytensor; print(pytensor.config.blas__ldflags)"
-L/home/ricardo/miniconda3/envs/pytensor/lib -llapack -lblas -lcblas -lm -Wl,-rpath,/home/ricardo/miniconda3/envs/pytensor/lib
PYTENSOR_FLAGS='cxx=""' python -c "import pytensor; print(pytensor.config.blas__ldflags)"
WARNING (pytensor.tensor.blas): Using NumPy C-API based implementation for BLAS functions.

@lucianopaz
Copy link
Member Author

Mine doesn't seem to have changed:

That looks fine. To add on this, I tried out using a C compiler that pytensor seems to like but I don't have installed

PYTENSOR_FLAGS='cxx="icpc"' python -c "import pytensor; print(pytensor.config.blas__ldflags)"
~/repos/pytensor/pytensor/link/c/cmodule.py:2735: UserWarning: Pytensor cxx failed to communicate its search dirs. As a consequence, it might not be possible to automatically determine the blas link flags to use.
Command that was run: icpc -print-search-dirs
Output printed to stderr: /bin/sh: 1: icpc: not found

  warnings.warn(
WARNING (pytensor.tensor.blas): Using NumPy C-API based implementation for BLAS functions.

So I think that this PR is good to be merged

@lucianopaz lucianopaz merged commit eb552ee into pymc-devs:main Nov 20, 2023
@lucianopaz lucianopaz deleted the blas_no_cxx branch November 20, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working C-backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

blas IndexError: list index out of range error
3 participants