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

Handle windows conda library directories in default blas #517

Merged
merged 3 commits into from
Nov 24, 2023

Conversation

lucianopaz
Copy link
Contributor

Closes #515

Major / Breaking Changes

  • ...

New features

  • ...

Bugfixes

  • Fix conda C library detection on Windows

Documentation

  • ...

Maintenance

  • ...

Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

is that failing test a bug uncovered by a change in the graph?

pytensor/link/c/cmodule.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #517 (7943c3d) into main (91d3b7c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #517   +/-   ##
=======================================
  Coverage   80.82%   80.82%           
=======================================
  Files         162      162           
  Lines       46189    46194    +5     
  Branches    11288    11290    +2     
=======================================
+ Hits        37330    37335    +5     
  Misses       6633     6633           
  Partials     2226     2226           
Files Coverage Δ
pytensor/link/c/cmodule.py 55.52% <100.00%> (+0.18%) ⬆️

@lucianopaz
Copy link
Contributor Author

is that failing test a bug uncovered by a change in the graph?

The failing test happened because there were empty blas__ldflags

@ricardoV94
Copy link
Member

Isn't it good that the test actually failed?

@ricardoV94
Copy link
Member

Okay you changed the dprint one, that's fine

@lucianopaz
Copy link
Contributor Author

The other failing test was caused by having one extra line in the print statement: the warning you get at import when no blas headers are available. That doesn’t seem to be a valid failure to me

@lucianopaz
Copy link
Contributor Author

Just to be clear, this doesn’t close #508. That one looks more like a nixOS problem with libraries and I need more info to address it

@ricardoV94
Copy link
Member

Yeah, I just wonder if we will be blind about regressions, not that this test should have that responsibility

@ricardoV94
Copy link
Member

@michaelosthege can you locally try on your Windows machine?

>>python -c "import pytensor; print(pytensor.config.blas__ldflags)"

@michaelosthege
Copy link
Member

michaelosthege commented Nov 24, 2023

@michaelosthege can you locally try on your Windows machine?

>>python -c "import pytensor; print(pytensor.config.blas__ldflags)"

-L"C:\Users\osthege\AppData\Local\mambaforge\envs\ptenv\Library\bin" -L"C:\Users\osthege\AppData\Local\mambaforge\envs\ptenv" -llapack -lblas -lcblas -lm -Wl,-rpath,C:\Users\osthege\AppData\Local\mambaforge\envs\ptenv\libs

environment was created by

mamba create -n ptenv "python=3.10" pytensor -y

followed by pip install -e . in this branch

@ricardoV94 ricardoV94 added bug Something isn't working C-backend labels Nov 24, 2023
@ricardoV94 ricardoV94 merged commit 5f84027 into pymc-devs:main Nov 24, 2023
53 checks passed
@lucianopaz lucianopaz deleted the conda_windows branch November 24, 2023 12:00
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 warning on Windows builds on conda-forge
5 participants