Remove duplicate data from summary#114
Conversation
The FlowResults class was renamed SimFlowResults a while back to differentiate it from the results from the other flows. However it looks like the old version remained due to commit rebase issues. Signed-off-by: James McCorrie <james.mccorrie@lowrisc.org>
At the moment the `SimResultsSummary` contains both the summary and also the full results. This creates duplicated data within the JSON data files. This commit separates out the summary data from the full detailed results. Which means that the block level reports JSON files will now be the primary source of the data for the detailed results. The summary JSON will now no longer replicate the same data. Signed-off-by: James McCorrie <james.mccorrie@lowrisc.org>
AlexJones0
left a comment
AlexJones0
left a comment
There was a problem hiding this comment.
Thanks for the work on this @machshev, it definitely seems like an improvement.
I'm happy for this to go in when the Python versioning issue is addressed.
| return { | ||
| flow: SimFlowResults.load( | ||
| path=base_path / f"{flow}.json", | ||
| ) | ||
| for flow in self.flow_results | ||
| } |
There was a problem hiding this comment.
Maybe this should be more robust against deleted results? It might be sensible to log.warn on any block report paths that are missing, and omit them from the returned mapping?
There was a problem hiding this comment.
I'd probably lean more towards an hard error unless we have a use case for something more relaxed. If we relax it then this could mask a bigger issues with data retention or perhaps the path provided is incorrect.
The summary JSON can be used standalone, but if we are trying to get the full set of results then I would expect a full set of results. They must have been available at the time they were generated.
src/dvsim/sim/report.py
Outdated
| relative = Path(self.relative_to) if self.relative_to is not None else Path.cwd() | ||
| block_report = self.html_link_base / f"{file_name}.html" | ||
| html_report_path = block_report.relative_to(relative) | ||
| html_report_path = block_report.relative_to(relative, walk_up=True) |
There was a problem hiding this comment.
I appreciate the fix, I wasn't aware that relative_to didn't do this walk-up behaviour by default.
It seems correct to me, but I think this walk_up parameter was only added in Python 3.12, so this will crash when run with e.g. Python 3.10.19, which I'm using locally.
There was a problem hiding this comment.
Yes it was a surprise to me as well that relative_to didn't work with non sub-paths. Looks like os.path.relpath supports the same functionality. It's a shame we have to revert back to os.path for this.
There was a problem hiding this comment.
Updated with the helper which uses Path as discussed. This adds unitttests for the functionality, which means we should catch any issues with Python version divergences as CI runs the unit tests against all supported Python versions.
machshev
force-pushed
the
split-summary
branch
2 times, most recently
from
80bfd04 to
61765ad
Compare
Where the project root is overridden with `--proj-root` and scratch directory is overridden with `--scratch` the HTML report path is not necessarily going to be a subdirectory of the project root. The default `Path.relative_to(...)` raises an exception if the resulting `Path` is `../some/parent/dir`. Signed-off-by: James McCorrie <james.mccorrie@lowrisc.org>
AlexJones0
left a comment
AlexJones0
left a comment
There was a problem hiding this comment.
Thanks for adding the walk_up replacement and adding a test for it! LGTM.
2 participants
Prepare for adding support for a dashboard.