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

Fix supplying (base_)compiledir as str #927

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

Armavica
Copy link
Member

Description

Fixes the bug signaled in https://discourse.pymc.io/t/changing-the-base-compiled-in-pytensor/14764

Related Issue

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

Copy link

codecov bot commented Jul 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.38%. Comparing base (a6b9585) to head (418fdf8).
Report is 102 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #927      +/-   ##
==========================================
+ Coverage   81.37%   81.38%   +0.01%     
==========================================
  Files         170      170              
  Lines       46845    46862      +17     
  Branches    11422    11423       +1     
==========================================
+ Hits        38118    38139      +21     
+ Misses       6545     6542       -3     
+ Partials     2182     2181       -1     
Files with missing lines Coverage Δ
pytensor/configdefaults.py 72.67% <100.00%> (+1.24%) ⬆️

... and 1 file with indirect coverage changes

@Armavica Armavica requested a review from ricardoV94 July 13, 2024 03:18
@Armavica Armavica added the bug Something isn't working label Jul 13, 2024
@ricardoV94 ricardoV94 requested a review from maresb July 13, 2024 06:05
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.

Looks good to me. I committed a few minor changes to test_config.py:

  • use the tmp_path fixture instead of mkdtemp
  • separate test functions for separate cases

Feel free to revert if you don't like it.

I wonder if we could actually add tests where we set compiledir and then actually compile something, but if it's just for the C backend then it's probably not worthwhile. Also we're not testing ~-expansion, but also probably not worth it.

Thanks @Armavica!

@ricardoV94 ricardoV94 merged commit 72c6a81 into pymc-devs:main Jul 15, 2024
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants