Skip to content

Conversation

lucianopaz
Copy link
Member

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?

@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
Member 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
Member 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
Member 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
@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