-
Notifications
You must be signed in to change notification settings - Fork 14
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.
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
add representation methods and encoders #70
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Arnaud, I am wondering what the end game is here. Could you describe it in a comment or commit message? It'll allow the reviews to be better directed.
The encoders are useful to store objects on disk between multiple calls to a program. This is useful in some cases where you need to recover the previous state of your run. |
e934207
to
cfa5eea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. It has some merge conflicts right now with latest in master branch. @arfio, could you please rebase it?
The encoders are useful to store objects on disk between multiple calls to a program. This is useful in some cases where you need to recover the previous state of your run. The representations implementation is very useful to print the results of the requests to know what you got. It is very useful for debugging and testing out things when developping. Signed-off-by: Arnaud Fiorini <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge with might
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. When testing I encountered some issues which I ask you to address.
I had to add some code to test the additions. Is there a way to provide some tests?
(please remember when runny pytest, the trace server workspace is wiped out, so run it on a new workspace)
if isinstance(obj.model, GenericResponse): | ||
return { | ||
'model': GenericResponseEncoder().default(obj.model), | ||
RESPONSE_STATUS_KEY : obj.status, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be obj.status_code
. (see below for details).
elif isinstance(obj.model, OutputDescriptorSet): | ||
return { | ||
'model': [OutputDescriptorEncoder().default(output_descriptor) for output_descriptor in obj.model.descriptors], | ||
RESPONSE_STATUS_KEY: obj.status, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be obj.status_code
. Otherwise I get an error when doing json.dumps(response, cls=TspClientResponseEncoder)
for list_outputs
:
File "/home/user/git/tsp-python-client/./tsp_cli_client", line 366, in <module>
json.dumps(response, cls=TspClientResponseEncoder)
File "/usr/lib/python3.10/json/__init__.py", line 238, in dumps
**kw).encode(obj)
File "/usr/lib/python3.10/json/encoder.py", line 199, in encode
chunks = self.iterencode(o, _one_shot=True)
File "/usr/lib/python3.10/json/encoder.py", line 257, in iterencode
return _iterencode(o, 0)
File "/home/user/git/tsp-python-client/tsp/tsp_client_response.py", line 78, in default
RESPONSE_STATUS_KEY: obj.status,
AttributeError: 'TspClientResponse' object has no attribute 'status'
else: | ||
return { | ||
'model': obj.model, | ||
RESPONSE_STATUS_KEY: obj.status, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be obj.status_code.
parameters: Query object | ||
:return: :class:`TspClientResponse <Trace>` object | ||
:rtype: TspClientResponse | ||
''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameter max_depth
and filter
are not described in doc and should be added.
Besides, it seems that these are new parameters in the TSP specification. It seems unrelated to this PR. Should it be part of a separate PR?
@@ -47,3 +52,36 @@ def __init__(self, model, status, status_text): | |||
|
|||
# The status message | |||
self.status_text = status_text | |||
|
|||
self.size = size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size not used in the PR. Is this for debug purposes as well?
series.print(array_print) | ||
|
||
def __repr__(self): | ||
return f'XYModel(title={self.title}, series={self.series})' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the print method was removd and hence calling ./tsp_cli_client --get-xy OUTPUT_ID --uuid UUID --items ITEMS --time-range START END NUM_TIMES
fails:
Traceback (most recent call last):
File "/home/user/git/tsp-python-client/./tsp_cli_client", line 423, in <module>
xyModel.print(array_print=True)
AttributeError: 'XYModel' object has no attribute 'print'
# Convert TspClientResponse to a dictionary | ||
if isinstance(obj.model, GenericResponse): | ||
return { | ||
'model': GenericResponseEncoder().default(obj.model), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I add json.dumps(response, cls=TspClientResponseEncoder) for get_timegraph_tree
I get the following error:
Traceback (most recent call last):
File "/usr/lib/python3.10/runpy.py", line 196, in _run_module_as_main
return _run_code(code, main_globals, None,
File "/usr/lib/python3.10/runpy.py", line 86, in _run_code
exec(code, run_globals)
File "/home/user/.vscode-oss/extensions/ms-python.python-2024.2.1-universal/pythonFiles/lib/python/debugpy/adapter/../../debugpy/launcher/../../debugpy/__main__.py", line 39, in <module>
cli.main()
File "/home/user/.vscode-oss/extensions/ms-python.python-2024.2.1-universal/pythonFiles/lib/python/debugpy/adapter/../../debugpy/launcher/../../debugpy/../debugpy/server/cli.py", line 430, in main
run()
File "/home/user/.vscode-oss/extensions/ms-python.python-2024.2.1-universal/pythonFiles/lib/python/debugpy/adapter/../../debugpy/launcher/../../debugpy/../debugpy/server/cli.py", line 284, in run_file
runpy.run_path(target, run_name="__main__")
File ["/home/user/.vscode-oss/extensions/ms-python.python-2024.2.1-universal/pythonFiles/lib/python/debugpy/_vendored/pydevd/_pydevd_bundle/pydevd_runpy.py",](url) line 321, in run_path
return _run_module_code(code, init_globals, run_name,
File "/home/user/.vscode-oss/extensions/ms-python.python-2024.2.1-universal/pythonFiles/lib/python/debugpy/_vendored/pydevd/_pydevd_bundle/pydevd_runpy.py", line 135, in _run_module_code
_run_code(code, mod_globals, init_globals,
File "/home/user/.vscode-oss/extensions/ms-python.python-2024.2.1-universal/pythonFiles/lib/python/debugpy/_vendored/pydevd/_pydevd_bundle/pydevd_runpy.py", line 124, in _run_code
exec(code, run_globals)
File "tsp_cli_client", line 390, in <module>
__get_tree(options.uuid, options.get_timegraph_tree, "TIME_GRAPH")
File "tsp_cli_client", line 117, in __get_tree
json.dumps(response, cls=TspClientResponseEncoder)
File "/usr/lib/python3.10/json/__init__.py", line 238, in dumps
**kw).encode(obj)
File "/usr/lib/python3.10/json/encoder.py", line 199, in encode
chunks = self.iterencode(o, _one_shot=True)
File "/usr/lib/python3.10/json/encoder.py", line 257, in iterencode
return _iterencode(o, 0)
File "/home/user/git/tsp-python-client/tsp/tsp_client_response.py", line 71, in default
'model': GenericResponseEncoder().default(obj.model),
File "/home/user/git/tsp-python-client/tsp/response.py", line 127, in default
model = EntryModelEncoder().default(obj.model)
File "/home/user/git/tsp-python-client/tsp/entry_model.py", line 69, in default
'entries': [TimeGraphEntryEncoder().default(entry) if isinstance(entry, TimeGraphEntry) else EntryEncoder().default(entry) for entry in obj.entries]
File "/home/user/git/tsp-python-client/tsp/entry_model.py", line 69, in <listcomp>
'entries': [TimeGraphEntryEncoder().default(entry) if isinstance(entry, TimeGraphEntry) else EntryEncoder().default(entry) for entry in obj.entries]
File "/home/user/git/tsp-python-client/tsp/time_graph_model.py", line 206, in default
'start': obj.start_time,
AttributeError: 'TimeGraphEntry' object has no attribute 'start_time'
f', x_axis={self.x_axis}' if hasattr(self, 'x_axis') else '' | ||
f', y_axis={self.y_axis}' if hasattr(self, 'y_axis') else '' | ||
f', x_values={self.x_values}' if hasattr(self, 'x_values') else '' | ||
f', y_values={self.y_values}' if hasattr(self, 'y_values') else '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
series_name
and series_id
are not printed anymore
When running, for example (after fixing the bug in tsp_cli_client)
./tsp_cli_client --get-xy org.eclipse.tracecompass.analysis.os.linux.core.cpuusage.CpuUsageDataProvider --items 0 1 2 --uuid d280b5ab-b0ff-38cf-b3e1-d33a0d56d1a3 --time-range 93083211504 93084211504 10`
I get the following output. It seems that the y_values
are not printed even if they are in the returned model:
XYModel(title=CPU Usage, series=[, x_values=[93083211504, 93083322615, 93083433726, 93083544837, 93083655948, 93083767059, 93083878170, 93083989281, 93084100392, 93084211503, 93084211504], , x_values=[93083211504, 93083322615, 93083433726, 93083544837, 93083655948, 93083767059, 93083878170, 93083989281, 93084100392, 93084211503, 93084211504], , x_values=[93083211504, 93083322615, 93083433726, 93083544837, 93083655948, 93083767059, 93083878170, 93083989281, 93084100392, 93084211503, 93084211504]])
Besides, I'm getting 3x the following sonarlint warnings:
Extract this nested conditional expression into an independent statement. [+1 location]sonarlint(python:S3358)
} | ||
else: | ||
return { | ||
'model': obj.model, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some modules don't have a encoder implemented yet. For example, when running --list-experiments
and do a json.dumps on it then I get an error:
TypeError: Object of type ExperimentSet is not JSON serializable
The same for list-traces
:
TypeError: Object of type TraceSet is not JSON serializable
This method adds encoders to be able to easily store the responses in json format.
It also adds repr() method to used models to be able to print and easily see what is contained in a response.