search

FLAMEGPU / FLAMEGPU2

FLAME GPU 2 is a GPU accelerated agent based modelling framework for CUDA C++ and Python
https://flamegpu.com
MIT License
106 stars 22 forks source link

Swig warnings, options and test fixes #1247

Closed ptheywood closed 1 week ago

ptheywood commented 1 week ago
ptheywood commented 1 week ago

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

ptheywood commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

That line number feels wrong

https://github.com/FLAMEGPU/FLAMEGPU2/blob/722a77910931f076670d34978e04c70efb0aac64/tests/test_cases/runtime/agent/host_reduction/test_transform_reduce.cu#L59

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.

https://github.com/FLAMEGPU/FLAMEGPU2/blob/722a77910931f076670d34978e04c70efb0aac64/tests/test_cases/runtime/agent/host_reduction/test_transform_reduce.cu#L72

ptheywood commented 1 week ago

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

Robadob commented 1 week ago

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

Ignore it then 🤷‍♂️

ptheywood commented 1 week ago

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 https://github.com/FLAMEGPU/FLAMEGPU2/pull/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 commented 1 week ago

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