Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: handle non json serializable types #310

Merged
merged 1 commit into from
May 28, 2024
Merged

fix: handle non json serializable types #310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 81 additions & 37 deletions _appmap/test/data/pytest/expected/pytest.appmap.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,95 +8,133 @@
"name": "appmap",
"url": "https://github.com/applandinc/appmap-python"
},
"source_location": "test_simple.py:5",
"name": "hello world",
"feature": "Hello world",
"app": "Simple",
"recorder": {
"name": "pytest",
"type": "tests"
},
"source_location": "test_simple.py:5",
"name": "hello world",
"feature": "Hello world",
"test_status": "succeeded"
},
"events": [
{
"defined_class": "simple.Simple",
"method_id": "hello_world",
"path": "simple.py",
"lineno": 8,
"static": false,
"receiver": {
"class": "simple.Simple",
"kind": "req",
"value": "<simple.Simple object at 0xabcdef>",
"name": "self",
"value": "<simple.Simple object at 0xabcdef>"
"class": "simple.Simple"
},
"parameters": [],
"id": 1,
"event": "call",
"thread_id": 1
"thread_id": 1,
"defined_class": "simple.Simple",
"method_id": "hello_world",
"path": "simple.py",
"lineno": 16
},
{
"static": false,
"receiver": {
"kind": "req",
"value": "<simple.Simple object at 0xabcdef>",
"name": "self",
"class": "simple.Simple"
},
"parameters": [],
"id": 2,
"event": "call",
"thread_id": 1,
"defined_class": "simple.Simple",
"method_id": "hello",
"method_id": "get_non_json_serializable",
"path": "simple.py",
"lineno": 2,
"lineno": 13
},
{
"static": false,
"receiver": {
"class": "simple.Simple",
"kind": "req",
"value": "<simple.Simple object at 0xabcdef>",
"name": "self",
"value": "<simple.Simple object at 0xabcdef>"
"class": "simple.Simple"
},
"parameters": [],
"id": 2,
"id": 3,
"event": "call",
"thread_id": 1
"thread_id": 1,
"defined_class": "simple.Simple",
"method_id": "hello",
"path": "simple.py",
"lineno": 7
},
{
"return_value": {
"class": "builtins.str",
"value": "'Hello'"
"value": "'Hello'",
"class": "builtins.str"
},
"parent_id": 2,
"id": 3,
"parent_id": 3,
"id": 4,
"event": "return",
"thread_id": 1
},
{
"defined_class": "simple.Simple",
"method_id": "world",
"path": "simple.py",
"lineno": 5,
"static": false,
"receiver": {
"class": "simple.Simple",
"kind": "req",
"value": "<simple.Simple object at 0xabcdef>",
"name": "self",
"value": "<simple.Simple object at 0xabcdef>"
"class": "simple.Simple"
},
"parameters": [],
"id": 4,
"id": 5,
"event": "call",
"thread_id": 1,
"defined_class": "simple.Simple",
"method_id": "world",
"path": "simple.py",
"lineno": 10
},
{
"return_value": {
"value": "'world!'",
"class": "builtins.str"
},
"parent_id": 5,
"id": 6,
"event": "return",
"thread_id": 1
},
{
"return_value": {
"class": "builtins.str",
"value": "'world!'"
"value": "{0: 'Hello', 1: 'world!'}",
"class": "builtins.dict",
"properties": [
{
"name": "0",
"class": "builtins.str"
},
{
"name": "1",
"class": "builtins.str"
}
],
"size": 2
},
"parent_id": 4,
"id": 5,
"parent_id": 2,
"id": 7,
"event": "return",
"thread_id": 1
},
{
"return_value": {
"class": "builtins.str",
"value": "'Hello world!'"
"value": "'Hello world!'",
"class": "builtins.str"
},
"parent_id": 1,
"id": 6,
"id": 8,
"event": "return",
"thread_id": 1
}
Expand All @@ -110,22 +148,28 @@
"name": "Simple",
"type": "class",
"children": [
{
"name": "get_non_json_serializable",
"type": "function",
"location": "simple.py:13",
"static": false
},
{
"name": "hello",
"type": "function",
"location": "simple.py:2",
"location": "simple.py:7",
"static": false
},
{
"name": "hello_world",
"type": "function",
"location": "simple.py:8",
"location": "simple.py:16",
"static": false
},
{
"name": "world",
"type": "function",
"location": "simple.py:5",
"location": "simple.py:10",
"static": false
}
]
Expand Down
11 changes: 10 additions & 1 deletion _appmap/test/data/pytest/simple.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,18 @@
import numpy

zero = numpy.int64(0)
one = numpy.int64(1)

class Simple:
def hello(self):
return "Hello"

def world(self):
return "world!"

def get_non_json_serializable(self):
return { zero: self.hello(), one: self.world() }

def hello_world(self):
return "%s %s" % (self.hello(), self.world())
result = self.get_non_json_serializable()
return "%s %s" % (result[zero], result[one])
20 changes: 20 additions & 0 deletions _appmap/test/test_generation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import json
import pytest

import numpy as np

from _appmap.generation import AppMapEncoder


@pytest.mark.appmap_enabled
@pytest.mark.usefixtures("with_data_dir")
Expand Down Expand Up @@ -48,3 +53,18 @@ def check_comment(self, to_dict):
return ret

verify_example_appmap(check_comment, "instance_method")

class TestAppMapEncoder:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@zermelo-wisen zermelo-wisen May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

def test_np_int64_type(self):
data = {
"value": np.int64(42),
}
json_str = json.dumps(data, cls=AppMapEncoder)
assert '{"value": "42"}' == json_str

def test_np_array_type(self):
data = {
"value": np.array([0, 1, 2, 3])
}
json_str = json.dumps(data, cls=AppMapEncoder)
assert '{"value": "[0 1 2 3]"}' == json_str
3 changes: 2 additions & 1 deletion requirements-dev.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ pytest-django<4.8
fastapi
httpx
sqlalchemy
debugpy
debugpy
numpy
2 changes: 2 additions & 0 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ allowlist_externals =
deps=
poetry
web: {[web-deps]deps}
py38: numpy==1.24.4
py3{9,10,11,12}: numpy >=1.26
flask2: Flask >= 2.0, <3.0
django3: Django >=3.2, <4.0
sqlalchemy1: sqlalchemy >=1.4.11, <2.0
Expand Down
Loading