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.

Sign up for GitHub

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

Jump to bottom

Fix C++11 narrowing error on Mac OS #972

Merged
merged 1 commit into from
Feb 17, 2023
Merged

Fix C++11 narrowing error on Mac OS #972

merged 1 commit into from
Feb 17, 2023

Conversation

dgerlanc
Copy link
Contributor

@dgerlanc dgerlanc commented May 31, 2022 •
edited
Loading

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 •
edited
Loading

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
brandonwillard
brandonwillard reviewed Jun 10, 2022
View reviewed changes
aesara/configdefaults.py Outdated
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?

@dgerlanc dgerlanc force-pushed the fix-narrowing-error branch from 648554c to 4418440 Compare June 14, 2022 15:16
@dgerlanc dgerlanc force-pushed the fix-narrowing-error branch from 4418440 to 6530d34 Compare October 9, 2022 23:24
@dgerlanc dgerlanc force-pushed the fix-narrowing-error branch 2 times, most recently from 3785e94 to 9dc32b5 Compare November 30, 2022 01:29
@dgerlanc dgerlanc force-pushed the fix-narrowing-error branch from 9dc32b5 to 2f38704 Compare December 2, 2022 21:03
brandonwillard
brandonwillard previously approved these changes Dec 2, 2022
View reviewed changes
@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 •
edited
Loading

@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 mentioned this pull request Feb 15, 2023
Closed
@brandonwillard brandonwillard linked an issue Feb 15, 2023 that may be closed by this pull request
Gcc_flag issue with Max os aesara-devs/aemcmc#106
Closed
@brandonwillard brandonwillard requested a review from rlouf February 15, 2023 22:50
@brandonwillard brandonwillard marked this pull request as ready for review February 15, 2023 22:51
brandonwillard
brandonwillard previously approved these changes Feb 15, 2023
View reviewed changes
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 force-pushed the fix-narrowing-error branch from 40d12ff to 7306ee4 Compare February 17, 2023 01:56
@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
Reviewers

@brandonwillard brandonwillard brandonwillard left review comments

@rlouf rlouf Awaiting requested review from rlouf

Assignees
No one assigned
Labels
bug Something isn't working C-backend MacOS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Gcc_flag issue with Max os Narrowing error on macOS
3 participants
@dgerlanc @twiecki @brandonwillard