FLAMEGPU / FLAMEGPU2

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

[Test Failure] Swig RunPlan != can't compare with None? #1233

Open Robadob opened 2 days ago

Robadob commented 2 days ago

Couple of Python test failures (debug/windows) whilst investigating the SWIG 4.2.1 issue, note this is with the Python wheel built with SWIG 4.2.1. Although, I've built it with 4.0.2 and received the same failures.

Not immediately clear their cause, these weren't touched in the recent bugfix so that's a coincidence.

====================================================== FAILURES =======================================================
____________________________________________ TestRunPlan.test_constructor _____________________________________________

self = <test_RunPlan.TestRunPlan testMethod=test_constructor>

    def test_constructor(self):
        # Create a model
        model = pyflamegpu.ModelDescription("test")
        plan = None
        plan = pyflamegpu.RunPlan(model)
>       assert plan != None

simulation\test_RunPlan.py:13:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <pyflamegpu.pyflamegpu.RunPlan; proxy of <Swig Object of type 'flamegpu::RunPlan *' at 0x0000022AD963CB40> >
rhs = None

    def __ne__(self, rhs):
>       return _pyflamegpu.RunPlan___ne__(self, rhs)
E       ValueError: invalid null reference in method 'RunPlan___ne__', argument 2 of type 'flamegpu::RunPlan const &'

..\..\build\lib\Debug\python\venv\Lib\site-packages\pyflamegpu\pyflamegpu.py:16808: ValueError
_________________________________________ TestRunPlanVector.test_constructor __________________________________________

self = <test_RunPlanVector.TestRunPlanVector testMethod=test_constructor>

    def test_constructor(self):
        # Create a model
        model = pyflamegpu.ModelDescription("test")
        # Declare a pointer
        plans = None
        # Use New
        initialLength = 4
        plans = pyflamegpu.RunPlanVector(model, initialLength)
>       assert plans != None

simulation\test_RunPlanVector.py:26:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <pyflamegpu.pyflamegpu.RunPlanVector; proxy of <Swig Object of type 'flamegpu::RunPlanVector *' at 0x0000022AD95E5BC0> >
rhs = None

    def __ne__(self, rhs):
>       return _pyflamegpu.RunPlanVector___ne__(self, rhs)
E       ValueError: invalid null reference in method 'RunPlanVector___ne__', argument 2 of type 'flamegpu::RunPlanVector const &'

..\..\build\lib\Debug\python\venv\Lib\site-packages\pyflamegpu\pyflamegpu.py:18426: ValueError
=============================================== short test summary info ===============================================
FAILED simulation/test_RunPlan.py::TestRunPlan::test_constructor - ValueError: invalid null reference in method 'RunPlan___ne__', argument 2 of type 'flamegpu::RunPlan const &'
FAILED simulation/test_RunPlanVector.py::TestRunPlanVector::test_constructor - ValueError: invalid null reference in method 'RunPlanVector___ne__', argument 2 of type 'flamegpu::RunPlanVector co...
================================ 2 failed, 674 passed, 12 skipped in 727.95s (0:12:07) ================================
Robadob commented 2 days ago

Tbh the actual test looks a bit janky, it should probably just be a test that runplan constructor doesn't throw an exception, this equivalence doesn't really match the new/delete behaviour of the C test.

Though arguably, I'd prefer swig to detect when a class of wrong type or None is passed to equality and always return false. This same error could be present throughout the codebase. Can't find any obvious documentation for how SWIG should handle this edge-case, and not clear in the slightest how it ever didn't occur given I managed to reproduce it with SWIG 4.0.2.

ptheywood commented 1 day ago

Can confirm this error is occuring under linux too, with Python 3.10 and Swig 4.2.1, however a local build using Swig 4.0.2 passess these tests successfully, implying it might be a change in swig?

From users writing python, it should be possibel to compare to None without haveing to also check the type first, so it is something we should try and resolve so that the test (which has been unchanged in 3 years since it was originally written) passes.

The rc1 wheels from our wheel house which were built with 4.0.2 also still pass the test.