-
-
Notifications
You must be signed in to change notification settings - Fork 379
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
Skip invalid json files in ReportBuilder #384
Changes from 11 commits
5b28214
79f7df5
df42ecf
62ac0a8
37541f5
d1c13c1
8e719de
c813ee1
72a2c47
f0d92bb
2dcdfd0
41c31b0
1f2afca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -47,6 +47,7 @@ def __init__( | |||||||||||||||||||||||
self.align = align | ||||||||||||||||||||||||
self.minimize_memory = minimize_memory | ||||||||||||||||||||||||
self.jsons: List[Dict] = [] | ||||||||||||||||||||||||
self.invalid_json_paths: List[str] = [] | ||||||||||||||||||||||||
self.json_loaded = False | ||||||||||||||||||||||||
self.final_messages: List[Tuple[str, Dict]] = [] | ||||||||||||||||||||||||
if not isinstance(data, (dict, list, tuple)): | ||||||||||||||||||||||||
|
@@ -67,18 +68,28 @@ def load_jsons(self) -> None: | |||||||||||||||||||||||
self.jsons = [get_json(self.data)] | ||||||||||||||||||||||||
elif isinstance(self.data, (list, tuple)): | ||||||||||||||||||||||||
self.jsons = [] | ||||||||||||||||||||||||
self.invalid_json_paths = [] | ||||||||||||||||||||||||
for idx, j in enumerate(self.data): | ||||||||||||||||||||||||
if self.verbose > 0: | ||||||||||||||||||||||||
same_line_print(f"Loading trace data from processes {idx}/{len(self.data)}") | ||||||||||||||||||||||||
self.jsons.append(get_json(j)) | ||||||||||||||||||||||||
try: | ||||||||||||||||||||||||
self.jsons.append(get_json(j)) | ||||||||||||||||||||||||
except json.JSONDecodeError: | ||||||||||||||||||||||||
self.invalid_json_paths.append(j) | ||||||||||||||||||||||||
if len(self.invalid_json_paths) > 0: | ||||||||||||||||||||||||
self.final_messages.append(("corrupted_json", {"paths": self.invalid_json_paths})) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
def combine_json(self) -> None: | ||||||||||||||||||||||||
if self.verbose > 0: | ||||||||||||||||||||||||
same_line_print("Combining trace data") | ||||||||||||||||||||||||
if self.combined_json: | ||||||||||||||||||||||||
return | ||||||||||||||||||||||||
if not self.jsons: | ||||||||||||||||||||||||
if not self.jsons and not self.invalid_json_paths: | ||||||||||||||||||||||||
raise ValueError("Can't get report of nothing") | ||||||||||||||||||||||||
elif not self.jsons: | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you hate nested if so much :) . You are evaluating Just check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't notice that. And thank you for correcting my bad habit. |
||||||||||||||||||||||||
# print corrupted json files | ||||||||||||||||||||||||
self.print_messages() | ||||||||||||||||||||||||
raise ValueError("All json files are corrupted") | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing the exception message should be enough - this is a rather rare case, just separate that from "no files". |
||||||||||||||||||||||||
if self.align: | ||||||||||||||||||||||||
for one in self.jsons: | ||||||||||||||||||||||||
self.align_events(one["traceEvents"]) | ||||||||||||||||||||||||
|
@@ -213,3 +224,10 @@ def print_messages(self): | |||||||||||||||||||||||
color_print("OKGREEN", f"vizviewer \"{report_abspath}\"") | ||||||||||||||||||||||||
else: | ||||||||||||||||||||||||
color_print("OKGREEN", f"vizviewer {report_abspath}") | ||||||||||||||||||||||||
elif msg_type == "corrupted_json": | ||||||||||||||||||||||||
print("") | ||||||||||||||||||||||||
color_print("WARNING", "Found and ignored corrupted json file, you may lost some process data.") | ||||||||||||||||||||||||
color_print("WARNING", "Corrupted json file:") | ||||||||||||||||||||||||
for msg in msg_args["paths"]: | ||||||||||||||||||||||||
color_print("WARNING", f" {msg}") | ||||||||||||||||||||||||
print("") |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,9 @@ | |
import io | ||
import json | ||
import os | ||
import shutil | ||
import tempfile | ||
from unittest.mock import patch | ||
|
||
import viztracer | ||
from viztracer.report_builder import ReportBuilder | ||
|
@@ -70,6 +72,30 @@ def test_invalid_json(self): | |
with self.assertRaises(Exception): | ||
ReportBuilder([invalid_json_path], verbose=1) | ||
|
||
@patch('sys.stdout', new_callable=io.StringIO) | ||
def test_corrupted_json(self, mock_stdout): | ||
with tempfile.TemporaryDirectory() as tmpdir: | ||
corrupted_json_path = os.path.join(os.path.dirname(__file__), "data", "fib.py") | ||
valid_json_path = os.path.join(os.path.dirname(__file__), "data", "multithread.json") | ||
corrupted_json_file = shutil.copy(corrupted_json_path, os.path.join(tmpdir, "corrupted.json")) | ||
valid_json_file = shutil.copy(valid_json_path, os.path.join(tmpdir, "valid.json")) | ||
rb = ReportBuilder([corrupted_json_file, valid_json_file], verbose=1) | ||
with io.StringIO() as s: | ||
rb.save(s) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should check for some printed messages here. This test only assures no exceptions raised. |
||
self.assertIn("Corrupted json file", mock_stdout.getvalue()) | ||
|
||
@patch('sys.stdout', new_callable=io.StringIO) | ||
def test_all_corrupted_json(self, mock_stdout): | ||
with tempfile.TemporaryDirectory() as tmpdir: | ||
corrupted_json_path = os.path.join(os.path.dirname(__file__), "data", "fib.py") | ||
corrupted_json_file = shutil.copy(corrupted_json_path, os.path.join(tmpdir, "corrupted.json")) | ||
rb = ReportBuilder([corrupted_json_file], verbose=1) | ||
with self.assertRaises(Exception) as context: | ||
with io.StringIO() as s: | ||
rb.save(s) | ||
self.assertEqual(str(context.exception), "All json files are corrupted") | ||
self.assertIn("Corrupted json file", mock_stdout.getvalue()) | ||
|
||
def test_combine(self): | ||
with tempfile.TemporaryDirectory() as tmpdir: | ||
file_path1 = os.path.join(tmpdir, "result1.json") | ||
|
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.
Do a full search on this PR and change all "corrupted" to "invalid".