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 C++11 narrowing error on Mac OS #972

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

dgerlanc
Copy link
Contributor

@dgerlanc dgerlanc commented May 31, 2022

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 dgerlanc added bug Something isn't working C-backend MacOS labels May 31, 2022
@dgerlanc dgerlanc force-pushed the fix-narrowing-error branch 2 times, most recently from 51989e0 to 237f0cd Compare May 31, 2022 20:50
@codecov
Copy link

codecov bot commented May 31, 2022

Codecov Report

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

❗ 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%) ⬆️
aesara/tensor/__init__.py 95.55% <0.00%> (-0.75%) ⬇️
aesara/tensor/math.py 90.42% <0.00%> (-0.27%) ⬇️
aesara/scalar/basic.py 79.04% <0.00%> (-0.13%) ⬇️
aesara/scalar/__init__.py 100.00% <0.00%> (ø)

@twiecki
Copy link
Contributor

twiecki commented May 31, 2022

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.

@dgerlanc
Copy link
Contributor Author

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?

@brandonwillard
Copy link
Member

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

@dgerlanc dgerlanc force-pushed the fix-narrowing-error branch 2 times, most recently from 98b4387 to 648554c Compare June 9, 2022 18:12
no_cpp_narrowing_flag = "-Wno-c++11-narrowing"
cxx_flags = config.gcc__cxxflags.split()
if sys.platform == "darwin":
if no_cpp_narrowing_flag not in cxx_flags:
Copy link
Member

Choose a reason for hiding this comment

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

We can do this check without splitting, no?

brandonwillard
brandonwillard previously approved these changes Dec 2, 2022
@dgerlanc
Copy link
Contributor Author

dgerlanc commented Dec 7, 2022

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?

@brandonwillard
Copy link
Member

brandonwillard commented Dec 8, 2022

@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
brandonwillard previously approved these changes Feb 15, 2023
Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

I made some updates. Anyone with an affected Mac should give it a try; otherwise, we can merge.

@dgerlanc dgerlanc merged commit 71e2f1e into aesara-devs:main Feb 17, 2023
@dgerlanc dgerlanc deleted the fix-narrowing-error branch February 18, 2023 14:51
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 MacOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gcc_flag issue with Max os Narrowing error on macOS
3 participants