Network message v2

[Robadob]

Copied from #883, working in a new branch to avoid breaking FRE project's dependency during development.

Remaining tasks:

• Update current ID code to not enforce contiguous IDs
• Validate IDs
• Fix traversal
  • Validate early, translate at the end
  • Update pbm/ipbm to construct keys/vals with idmap
  • fix tests
• Add device API edge src-dest to index method
• Duplicate host API to offer both index and ID (This will necessitate a host ID map)
• (more) tests
• python/rtc API tests (particularly around no edges)
• native python/codegen tests (api has change a touch since they were setup)
• Improve HostAPI auto sync
• docs (Environment Directed Graph FLAMEGPU2-docs#167)

[Robadob]

Todo

Agree whether vertex IDs can be "sparse" (@ptheywood in favour because everyone else does it, me not in favour because it adds an additional indirection (if not more) when accessing the graph via ID)

[Robadob]

Sparse IDs, limited by memory. Users encouraged to keep them contiguous to avoid memory issues.

[Robadob]

Hard stuff is all done (unless i find more edge cases along the way).

I've agreed with @ptheywood that we should offer both index and ID access API, and just document index as being preferable for performance.

Remaining tasks:

• device/host id/index api duplication+ tests
• rtc device api tests
• docs
• update fujitsu

Should be achievable before the end of next week.

[Robadob]

Current discussion is for HostAPI to emulate C++ std::map. Albeit with a fixed size

HostStaticVertexMap HostEnvironmentDirectedGraph::vertices()

User first must resize it with existing HostEnvironmentDirectedGraph::setVertexCount().

They can then insert and update elements, using operator[]. Similar to a regular std::map, or use iteration mechanics.

insert(), erase(), emplace() would not be supported to avoid the need to define a host vertex structure (which would essentially be a wrapper for std::map<std::string, std::any> 🤮).

[Robadob]

HostAPI refactor should be complete, with all tests working on C++/Py.

Going to make a start on the documentation.

[Robadob]

Final changes to do next week are adding python tests around device API edge cases and checking whether there needs to be some IO documentation added.

[ptheywood]

This feels like a bug that was fixed to do with seatbelts elsewhere? Not sure if this is the correct value, mid review. Should double check this works with and without seatbelts just incase.

(this is just a note mid-review so feel free to ignor for now, but doubt I'll get through all of this in one go).

[Robadob]

I think there was a bug related to this, that you couldn't build with seatbelts turned off. It was probably in fujitsu and has been fixed here (not enough context in 2 lines of code shown to check off-hand).

[ptheywood]

Looks like it should be ok / intentaional, but again should make sure we test with and without seatbelts.

[ptheywood]

I'd prefer CSR/CSC in the names not PBM / IPBM, but prolly not worth the effort to fix / just my bias from doing more graph stuff.

[Robadob]

Doesn't bother me if you wish to refactor, so long as "PBM" is in the doc comment for my comprehension.

[ptheywood]

As discussed I've reviewed this focussing on the headers, and only quickly reviewing the implemntation.

Generally looks good, just some docstrings haven't been updated to reflect changes to the API.

There's an absence of any python tests of the host portion of the graph api, with only codegen tests.
Could do with some python tests mirroring atleast some of the c++ tests just to make sure the interface is relatively nice to use in python. Ignore that, i'd ticked the viewed box on that file by accident

Also would prefer CSR/CSC used in place of PBM/IPBM, but it's not important enough to change.

Should make sure that betls and no belts still build, Think it should be fine, but there's a var moved out of an #ifdef which we had a similar bug for recently too.

Going to need a rebase too.

[ptheywood]

seatbelts=off builds fail without a rebase, due to the strings bug fixed elsewhere.

seatbelts=on builds pass all tests (c++ and python) under linux as it stands.

I think the conflict wants to be resolved as follows (and changed to avoid the py3.8? bug that has been fixed in master since) but I am not 100% on this, so not force pushing.

        if t.targets[0].id not in self._locals :
            # Special case, catch message.at() where a message is returned outside a message loop
            if hasattr(t.value, "func") and isinstance(t.value.func, ast.Attribute) and t.value.func.attr == 'at' :
                if t.value.func.value.id == self._input_message_var :
                    self._standalone_message_var.append(t.targets[0].id)
            # Special case, track which variables hold directed graph handles
            elif hasattr(t.value, "func") and isinstance(t.value.func, ast.Attribute) and t.value.func.attr == 'getDirectedGraph' :
                if t.value.func.value.value.id == "pyflamegpu" and t.value.func.value.attr == "environment" :
                    self._directed_graph_vars.append(t.targets[0].id)
            # Special case, definitions outside of agent fn are made const
            if self._indent == 0:
                self.write("constexpr ")

Edit: That results in a test failure, also needs the test changing to account for the addition of constexpr (I think just to the expected result, but not 100%)

[Robadob]

Windows Release SEATBELTS=OFF C & Python tests now pass.

[ptheywood]

Linux release seatbelts=ON C tests all pass post-rebase.