fix: handle non json serializable types

[zermelo-wisen]

Fixes #283.

• Adds numpy as a dev dependency. v1.24.4 is the latest version supporting Python 3.8.
• Changes tox.ini to support different numpy versions in Python 3.8 and 3.{9,10,11,12}
• Adds tests for AppMapEncoder ensuring objects containing members with numpy.int64 and numpy.array types can be serialized without errors.
• Modifies a test that generates an AppMap to cover the issue.

[apotterri]

One additional test to add. Also, the build is red, but maybe you already noticed that?

[apotterri]

This test is good. I think it would also be helpful, especially to understand the impact of the change, to update one of the tests that actually generates an AppMap. One of these two could be good:

appmap-python/_appmap/test/web_framework.py

Line 286 in e4b8d78

def test_can_record(self, data_dir, client):

, or https://github.com/getappmap/appmap-python/blob/master/_appmap/test/test_test_frameworks.py#L94

[zermelo-wisen]

Actually, I tried to create exactly this as a separate AppMap generation test but wasn't able to produce the expected error - test passes without the fix as well. Returning int64 typed values from or accepting them as arguments to recorded functions seems not to produce the error.

Here is a temp branch if you want to take a look at what am I missing there: https://github.com/getappmap/appmap-python/tree/temp/test-recording-with-numpy-int64

[apotterri]

It's hard to tell why this isn't reproducing the expected behavior, because it's only checking the length of the event array.

The reason I picked those other two test cases was because they'd requiring updating the expected AppMap data.

Rather than entirely new test case, can you try updating one of those tests I suggested, please?

[zermelo-wisen]

It was not possible to produce a non JSON serializable AppMap by simply introducing a numpy.int64 value as an event parameter or as a return value of an event, since all values are converted to strings with event.py::display_string while encoding their container Event.

While examining the seaborn project which had this problem, it turned out that the issue occurs when numpy.int64 is used as a key in a dict that is a parameter or a return value of a recorded function. These keys become the name attributes of properties while describing the value and the schema of the return value or the parameter. The name attributes are not checked or converted to strings in event.py, which leads to this error.

In addition to the direct tests for AppMapEncoder, I modified one of the existing tests you suggested to produce the AppMap that includes the recording of a function returning a dict with numpy.int64 keys.

[zermelo-wisen]

One additional test to add. Also, the build is red, but maybe you already noticed that?

Yes, thank you. I just canceled my review request :)

[apotterri]

Thanks for separating out the testing updates into a separate commit, this makes it easier to understand what you did. However, please combine them into a single commit after you remove appmap.log.

I like to use fixup! commits to accomplish this: https://git-scm.com/docs/git-commit/2.32.0#Documentation/git-commit.txt---fixupamendrewordltcommitgt .

[apotterri]

Delete this, please.

[zermelo-wisen]

Oh sorry...

[apotterri]

Just want to double-check on this: #283 was fixed by #313, right? Given that there aren't any changes to how data is serialized here, seems like the commit here should be tagged test:, not fix:. We can leave the statement that it fixes #283 (since this was the final step).

[zermelo-wisen]

Just want to double-check on this: #283 was fixed by #313, right? Given that there aren't any changes to how data is serialized here, seems like the commit here should be tagged test:, not fix:. We can leave the statement that it fixes #283 (since this was the final step).

You are right, I forgot that you already merged the fixing part with #313. I'm changing the commit message accordingly.