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

Do not use deprecated find_executable #1000

Merged
merged 4 commits into from
Oct 2, 2024

Conversation

ricardoV94
Copy link
Member

Closes #997

@@ -349,7 +349,7 @@ def add_compile_configvars():

# Try to find the full compiler path from the name
if param != "":
newp = find_executable(param)
newp = shutil.which(param)
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be slightly different between windows v linux v unix.

Also does shutil reference any aliases? Is that necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

(not blocking just curious)

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea tbh, I only followed the deprecation recommendation

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll have to check how it handles aliases, but shutil.which is supposed to handle symlinks besides regular files, and it should also be able to work with executable extensions. The difference between path handling between windows and unix systems doesn't look to be too bad.

Copy link
Contributor

@lucianopaz lucianopaz left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @ricardoV94

@@ -349,7 +349,7 @@ def add_compile_configvars():

# Try to find the full compiler path from the name
if param != "":
newp = find_executable(param)
newp = shutil.which(param)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll have to check how it handles aliases, but shutil.which is supposed to handle symlinks besides regular files, and it should also be able to work with executable extensions. The difference between path handling between windows and unix systems doesn't look to be too bad.

@lucianopaz
Copy link
Contributor

Oops, I hadn't looked at the failed tests. I'll have to dig a bit deeper into the difference between shutil.which and find_executables. If we're lucky, it might just be a simple formatting problem that we need to iron out between the outputs of the two functions.

@ricardoV94
Copy link
Member Author

Someone mentioned this failure in another PR so it might be unrelated actually

@lucianopaz
Copy link
Contributor

Someone mentioned this failure in another PR so it might be unrelated actually

Yeah, it looks like it started at some point in the last three days and it's hitting multiple PRs. It must be some environment change in the github actions container.

@ricardoV94
Copy link
Member Author

Someone mentioned this failure in another PR so it might be unrelated actually

Yeah, it looks like it started at some point in the last three days and it's hitting multiple PRs. It must be some environment change in the github actions container.

Any idea how to go about fixing it?

@lucianopaz
Copy link
Contributor

Any idea how to go about fixing it?

It’s hard without a local env that fails. It could be possible to open a dummy PR and try to debug via the CI workflows. These failures in the blas configuration are just a mess. I don’t know why it was originally programmed in a way that yields an invalid configuration by default when things fail during the blas checks

@ricardoV94
Copy link
Member Author

I don’t know why it was originally programmed in a way that yields an invalid configuration by default when things fail during the blas checks

What do you mean? On the CI we just want to make sure we are running against BLAS

@ricardoV94 ricardoV94 force-pushed the deprecated_find_executable branch 3 times, most recently from 9bbd19c to c5859d9 Compare October 2, 2024 11:46
Copy link
Contributor

@maresb maresb left a comment

Choose a reason for hiding this comment

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

Not sure about links but LGTM

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.74%. Comparing base (3e55a20) to head (a9ed0f9).
Report is 8 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1000   +/-   ##
=======================================
  Coverage   81.74%   81.74%           
=======================================
  Files         183      183           
  Lines       47724    47733    +9     
  Branches    11616    11616           
=======================================
+ Hits        39011    39020    +9     
- Misses       6518     6520    +2     
+ Partials     2195     2193    -2     
Files with missing lines Coverage Δ
pytensor/configdefaults.py 71.61% <100.00%> (ø)

... and 2 files with indirect coverage changes

@ricardoV94 ricardoV94 force-pushed the deprecated_find_executable branch 2 times, most recently from 1256fee to 7ae68ed Compare October 2, 2024 13:21
@ricardoV94 ricardoV94 merged commit 8a6e407 into pymc-devs:main Oct 2, 2024
60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning about find_executable deprecation
4 participants