aesara icon indicating copy to clipboard operation
aesara copied to clipboard

Fix C++11 narrowing error on Mac OS

Open dgerlanc opened this issue 3 years ago • 4 comments

Resolves #127

Previously, tests including the following would fail with the following errors:

FAILED tests/graph/test_compute_test_value.py::TestComputeTestValue::test_empty_elemwise - aesara.link.c.exceptions.CompileError: Compilation failed (return status=1):
FAILED tests/graph/test_compute_test_value.py::TestComputeTestValue::test_overided_function - aesara.link.c.exceptions.CompileError: Compilation failed (return status=1):
FAILED tests/graph/test_compute_test_value.py::TestComputeTestValue::test_scan_err1 - aesara.link.c.exceptions.CompileError: Compilation failed (return status=1):
FAILED tests/graph/test_compute_test_value.py::TestComputeTestValue::test_scan_err2 - aesara.link.c.exceptions.CompileError: Compilation failed (return status=1):

TODO: Add tests to verify modification of config.gcc__cxxflags

  • [ ] No gcc_cxxflags
    • Darwin -> Add -Wno-c++11-narrowing
    • Other sys.platform -> No-Op
  • [ ] Has gcc__cxxflags but no -Wno-c++11-narrowing
    • Darwin -> Add -Wno-c++11-narrowing and other flags preserved
    • Other platform -> other flags preserved
  • [ ] Has gcc_cxxflags including -Wno-c++11-narrowing
    • All platforms -> gcc__cxxflags remain the same

dgerlanc avatar May 31 '22 20:05 dgerlanc

Codecov Report

Merging #972 (40d12ff) into main (ead2c02) will decrease coverage by 0.02%. The diff coverage is 100.00%.

:exclamation: Current head 40d12ff differs from pull request most recent head 2bef458. Consider uploading reports for the commit 2bef458 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #972      +/-   ##
==========================================
- Coverage   74.75%   74.73%   -0.02%     
==========================================
  Files         194      194              
  Lines       49896    49880      -16     
  Branches    10553    10550       -3     
==========================================
- Hits        37298    37278      -20     
- Misses      10270    10273       +3     
- Partials     2328     2329       +1     
Impacted Files Coverage Δ
aesara/link/c/cmodule.py 52.32% <100.00%> (+0.27%) :arrow_up:
aesara/tensor/__init__.py 95.55% <0.00%> (-0.75%) :arrow_down:
aesara/tensor/math.py 90.42% <0.00%> (-0.27%) :arrow_down:
aesara/scalar/basic.py 79.04% <0.00%> (-0.13%) :arrow_down:
aesara/scalar/__init__.py 100.00% <0.00%> (ø)

codecov[bot] avatar May 31 '22 21:05 codecov[bot]

We do this by default in pymc: https://github.com/pymc-devs/pymc/blob/main/pymc/init.py#L36 it should be pretty safe to just always do that.

twiecki avatar May 31 '22 22:05 twiecki

I was going to make this a bit fancier by checking the existing flags gcc_cxxflags on a platform-dependent basis, and only adding the compatibility flags as necessary. I don't think it's an issue to have duplicate flags (if the user has already included them), but does add a bit of noise.

Still need to add tests to aesara whether we use the pymc code @twiecki linked to or my implementation. @brandonwillard Any preference?

dgerlanc avatar May 31 '22 22:05 dgerlanc

In general, we should try to apply OS/arch-specific settings only to said OS/arch's, whenever possible.

brandonwillard avatar Jun 03 '22 17:06 brandonwillard

I should probably check that we're on macOS and using the C backend. This probably doesn't apply with the numba backend.

Also, I need to add a test case for this.

@brandonwillard Is there an existing location for config-related test cases?

dgerlanc avatar Dec 07 '22 21:12 dgerlanc

@brandonwillard Is there an existing location for config-related test cases?

tests.test_config is for the general configuration classes, but there really isn't a place for testing specific config settings. Since this config setting is specific to C compilation, tests.link.c.test_basic is probably a suitable place.

brandonwillard avatar Dec 08 '22 01:12 brandonwillard