Skip to content

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