Swig warnings, options and test fixes #1247

ptheywood · 2024-11-18T17:26:21Z

Fixes tests and exception messages introduced in Graph visualisation would only work if CUDASimulation was explicitly init() #1243 / ba93b32
Suppress Swig deprecation notices when runnign via pytest (these are suppressed by regular python invocation by default)
Add several CMake options / cache variables to allow control of the required/fetched Swig version, to enable easier testing & downgrading on CI.
Add CMake Warnings if Swig 4.2.0 or 4.2.1 are found
Update CI to use Swig 4.1.0, rather than the any version available greater than 4.0.2
- This is to avoid Swig 4.2.0/4.2.1, without switching to the less tested 4.3.0
Fix sign comparison warning in the test suite only issued on windows since include changes from cpplint2

.github/workflows/Draft-Release.yml

swig/CMakeLists.txt

ptheywood · 2024-11-19T11:31:08Z

Force-pushed with mark_as_advanced added and using 4.0.2 consistently. Lint CI will still fail as I did not rebase.

Robadob

Only thing im wary of is the tests you've updated, as the tested code is somewhat brittle. Have they been tested on both Linux/Windows?

ptheywood · 2024-11-19T12:54:32Z

Only thing im wary of is the tests you've updated, as the tested code is somewhat brittle. Have they been tested on both Linux/Windows?

Linux only. They were already failing since #1243 so worst case they are still failing on windows (but are fixed on linux)

ptheywood · 2024-11-19T23:02:52Z

Windows non-wheel Draft-Release CI has failed due to a warning in the test suite, which regular windows CI does not build due to time required.

However yesterdays push of this branch was fine, and the changes made today should not impact that workflow at all...

Looks to be in files which have not been modified recently, so presumably a newly detected warning or internal in xutility?

Doesn't reproduce locally with my current MSVC version (14.41 not 14.42)

2024-11-19T12:09:43.3918647Z          Compiling CUDA source file ..\..\tests\test_cases\runtime\agent\host_reduction\test_mean_standarddeviation.cu...
2024-11-19T12:09:52.6167081Z          cmd.exe /C "C:\Users\runneradmin\AppData\Local\Temp\tmp0c04d68cc9d34bd498d1687f2d8e070e.cmd"
2024-11-19T12:09:52.8670182Z      1>CudaBuildCore:
2024-11-19T12:10:00.0173934Z          tmpxft_00000cfc_00000000-7_test_transform_reduce.compute_90.cudafe1.cpp
2024-11-19T12:10:08.3354496Z ##[error]     1>C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.42.34433\include\xutility(6206): error C2220: the following warning is treated as an error [D:\a\FLAMEGPU2\FLAMEGPU2\build\tests\tests.vcxproj]
2024-11-19T12:10:16.6516026Z ##[warning]     1>C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.42.34433\include\xutility(6206): warning C4389: '==': signed/unsigned mismatch [D:\a\FLAMEGPU2\FLAMEGPU2\build\tests\tests.vcxproj]
2024-11-19T12:10:21.4022662Z          C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.42.34433\include\xutility(6206): note: the template instantiation context (the oldest one first) is
2024-11-19T12:10:26.1532942Z          D:\a\FLAMEGPU2\FLAMEGPU2\tests\test_cases\runtime\agent\host_reduction\test_transform_reduce.cu(59): note: see reference to function template instantiation '__int64 std::count<std::_Array_iterator<_Ty,256>,uint32_t>(const _InIt,const _InIt,const unsigned int &)' being compiled
2024-11-19T12:10:26.6904807Z                  with
2024-11-19T12:10:27.1224528Z                  [
2024-11-19T12:10:34.4514057Z                      _Ty=int,
2024-11-19T12:10:43.2948342Z                      _InIt=std::_Array_iterator<int,256>
2024-11-19T12:10:43.5019023Z                  ]

Windows C++ test suite (CUDA 12.6) all passed (1126)
Pytest (CUDA 12.6, python 3.9) all passed (676).

Took a long time though due to jitify CUDA 12.6 perf.

Robadob · 2024-11-20T07:09:37Z

D:\a\FLAMEGPU2\FLAMEGPU2\tests\test_cases\runtime\agent\host_reduction\test_transform_reduce.cu(59)

That line number feels wrong

FLAMEGPU2/tests/test_cases/runtime/agent/host_reduction/test_transform_reduce.cu

Line 59 in 722a779

TEST_F(HostReductionTest, CustomTransformReduceDouble) {

Feels more like line 72, though only solution feels like some redundant cast of the std::count call. Or, possibily some dumb distinction between int32_t being used in places and int elsewhere, not sure why CI would be building with a non 32 bit int type.

FLAMEGPU2/tests/test_cases/runtime/agent/host_reduction/test_transform_reduce.cu

Line 72 in 722a779

    
           EXPECT_EQ(int32_t_out, std::count(inTransform.begin(), inTransform.end(), static_cast<int>(1)));

ptheywood · 2024-11-20T11:04:15Z

The (fixed) stanadalone windows tests workflow didn't encounter the warnings...

https://github.com/FLAMEGPU/FLAMEGPU2/actions/runs/11931553879

Edit: The commits being checked out in the workflows is differnet, as DraftRelease in this case is triggered on pull_request not push in this case (as that is what is important for PRs which will cause a release to occur), so is post-merge, which may be relevant. I.e. this is caused by a change in includes to fix linting.

I'll do the rebase so I can re-trigger the standalone to try and repro

This regression was introduced by an edge-case fix for MSVC's filepath implementation, in #1243 / ba93b32

pytest.ini appears to require being above the outermost test file

- CMake warning if SWIG 4.2.0 is found that it probably won't work - CMake warning if Swig 4.2.1 is found that ==/!= None won't work - add FLAMEGPU_SWIG_MINIMUM CMake cache variable to allow overriding of the minimum swig version (i.e. to force a newer version on CI) - add FLAMEGPU_SWIG_DOWNLOAD CMake cachce variable to allow overriding of the version of swig to download if required swig not found (mostly for CI) - add FLAMEGPU_SWIG_EXACT CMake option which switches from a minimum required Swig to an exact required swig, to allow downgrading (on CI)

…manylinux swig images 4.3.0 has not been tested as much, so falling back to an older version for now, and 4.1.0 errored on CI during install

Robadob · 2024-11-20T12:05:54Z

The (fixed) stanadalone windows tests workflow didn't encounter the warnings...

Ignore it then 🤷‍♂️

ptheywood · 2024-11-20T14:19:15Z

Reproduced the errors in the standalone workflow after a rebase so it includes the cpplint changes.

So it should be caused by one of the changes in #1245

I'll try to reproduce it locally when i'm back in Windows with that knowledge, and if so either resolve or just suppress it. I expect it must be caused by an include change, but not sure how to resolve that or narrow down which include is problematic so might just suppress it if I can't find a quick-ish fix.

ptheywood · 2024-11-20T18:21:16Z

That line number feels wrong

That was the line number after the merge with master (i.e. the cpplint fixes)

it's actually the line before (and commenting it out prevents the warning):

EXPECT_EQ(uint32_t_out, std::count(inTransform.begin(), inTransform.end(), static_cast<uint32_t>(1)));

The inclusion of <algorithm> pushed the lines down, and commenting that out prevents the warning from being triggered...

    std::array<int, TEST_LEN> inTransform;
    // .. 
    EXPECT_EQ(uint32_t_out, std::count(inTransform.begin(), inTransform.end(), static_cast<uint32_t>(1)));

So it's a valid warning here, as each int element is being compared to a uint32(1), just very weird that it's only triggered when has been included, and the action being triggered by pull_request threw us off by looking at the wrong method which has int for both

… test_transform_reduce.cu std::count was comparing a uint32_t against int elements in a std::array. Warning only issued by msvc when <algorithm> has been included as part of a lint fix

Robadob

Not looked at it detail, but should be fine.

ptheywood force-pushed the swig-4.2.x-warnings branch 2 times, most recently from b8d349b to 55f25ad Compare November 18, 2024 18:45

ptheywood mentioned this pull request Nov 18, 2024

2.0.0-rc.2 final changes #1248

Merged

11 tasks

mondus reviewed Nov 19, 2024

View reviewed changes

.github/workflows/Draft-Release.yml Outdated Show resolved Hide resolved

mondus reviewed Nov 19, 2024

View reviewed changes

swig/CMakeLists.txt Show resolved Hide resolved

ptheywood force-pushed the swig-4.2.x-warnings branch from 55f25ad to 722a779 Compare November 19, 2024 11:30

ptheywood requested review from mondus and Robadob November 19, 2024 11:30

Robadob previously approved these changes Nov 19, 2024

View reviewed changes

ptheywood dismissed Robadob’s stale review via 5d0dc01 November 20, 2024 10:31

ptheywood added 5 commits November 20, 2024 11:27

Fix: Fix tests and exception error messages for missing extensions

ff77079

This regression was introduced by an edge-case fix for MSVC's filepath implementation, in #1243 / ba93b32

Pytest: Use pytest.ini to suppress any deprecation warnings from Swig

08f24f6

pytest.ini appears to require being above the outermost test file

CI: Use Swig 4.0.2 to avoid issues with 4.2.1 currerntly provided by …

2893cba

…manylinux swig images 4.3.0 has not been tested as much, so falling back to an older version for now, and 4.1.0 errored on CI during install

CI: Fix mistake in manually executable Windows-Tests.yml

05aefc2

ptheywood force-pushed the swig-4.2.x-warnings branch from 5d0dc01 to 05aefc2 Compare November 20, 2024 11:27

Warning fix: MSVC sign comparisonm warning when algorithm included in…

cc6eb5f

… test_transform_reduce.cu std::count was comparing a uint32_t against int elements in a std::array. Warning only issued by msvc when <algorithm> has been included as part of a lint fix

ptheywood requested a review from Robadob November 21, 2024 14:37

Robadob approved these changes Nov 21, 2024

View reviewed changes

ptheywood merged commit 0f89e15 into master Nov 21, 2024
71 checks passed

ptheywood deleted the swig-4.2.x-warnings branch November 21, 2024 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swig warnings, options and test fixes #1247

Swig warnings, options and test fixes #1247

ptheywood commented Nov 18, 2024 •

edited

Loading

ptheywood commented Nov 19, 2024 •

edited

Loading

Robadob left a comment

ptheywood commented Nov 19, 2024 •

edited

Loading

ptheywood commented Nov 19, 2024 •

edited

Loading

Robadob commented Nov 20, 2024

ptheywood commented Nov 20, 2024 •

edited

Loading

Robadob commented Nov 20, 2024

ptheywood commented Nov 20, 2024

ptheywood commented Nov 20, 2024 •

edited

Loading

Robadob left a comment

Swig warnings, options and test fixes #1247

Swig warnings, options and test fixes #1247

Conversation

ptheywood commented Nov 18, 2024 • edited Loading

ptheywood commented Nov 19, 2024 • edited Loading

Robadob left a comment

Choose a reason for hiding this comment

ptheywood commented Nov 19, 2024 • edited Loading

ptheywood commented Nov 19, 2024 • edited Loading

Robadob commented Nov 20, 2024

ptheywood commented Nov 20, 2024 • edited Loading

Robadob commented Nov 20, 2024

ptheywood commented Nov 20, 2024

ptheywood commented Nov 20, 2024 • edited Loading

Robadob left a comment

Choose a reason for hiding this comment

ptheywood commented Nov 18, 2024 •

edited

Loading

ptheywood commented Nov 19, 2024 •

edited

Loading

ptheywood commented Nov 19, 2024 •

edited

Loading

ptheywood commented Nov 19, 2024 •

edited

Loading

ptheywood commented Nov 20, 2024 •

edited

Loading

ptheywood commented Nov 20, 2024 •

edited

Loading