Skip to content

Add analysis db mode - #236

Closed
vedithal-amd wants to merge 4 commits into
ROCm:developfrom
vedithal-amd:analysis-db
Closed

vedithal-amd wants to merge 4 commits into
ROCm:developfrom
vedithal-amd:analysis-db

Conversation

@vedithal-amd

@vedithal-amd vedithal-amd commented Aug 10, 2025 •

Copy link
Copy Markdown
Contributor

Analysis data dump

  • Add --db option to analyze mode which creates database file
    containiing analysis result

  • Add documentation on rocpd output format and analysis database file

  • Create sqlite3 database using object relation mapping (ORM) provided
    by sqlalchemy library

  • Fix metrics config to remove metrics marked as null, fix Unit header, add
    missing title

  • Add test cases to ensure analysis data dump works

  • Update CHANGELOG

jbonnell-amd pushed a commit that referenced this pull request Aug 11, 2025
* Documentation updates

* formatting

* Update using-rocprofv3.rst

* Update counter_collection_services.md

---------

Co-authored-by: srawat <120587655+SwRaw@users.noreply.github.com>

[ROCm/rocprofiler-sdk commit: 31b8f61]
@vedithal-amd
vedithal-amd marked this pull request as ready for review August 12, 2025 02:02
jrmadsen pushed a commit that referenced this pull request Aug 12, 2025
[ROCm/rocprofiler-systems commit: 92b636a]
@vedithal-amd
vedithal-amd requested a review from a team as a code owner August 13, 2025 05:48
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Aug 13, 2025
@vedithal-amd
vedithal-amd force-pushed the analysis-db branch 2 times, most recently from 0f22531 to 1d29c3e Compare August 13, 2025 22:22

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems to me duplicated somewhere else, is it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is debatable for me, "db" is a end-user level analysis mode. It more likes on the same level of "--save-dfs". Thought?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would like to skip analysis report generation and only focus on creating database. That is why I am not using CLI mode with --save-dfs option. Remember, its not only about saving metrics dfs. We also need to think about roofline, pc sampling information storage in database.

For the end user, they can still do both analyze --save-dfs and analyze --db <db_name>, so we are keeping it flexible.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2025

@feizheng10 feizheng10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some general comments:

  • Before removing the Grafana solution with Mongo DB. To reduce the confusion for the end users, we might want to explicitly claim there is no relationship for those 2 in the doc.
  • As a general design, or user view, db output and df csv output are just alternatives to save the output as user's choice, they are just data storage, not analysis mode.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd apologize here: we should have the doc for "--save-dfs" early.
Since you adding "data-dump" section here, I'd say we might want to include description for both db and csv format. And even more, the option could be in more unified fashion: --output [txt/csv/db]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed we should replace -o, --save-dfs, --db analysis report options with ``--output txt/csv/db`. I will make this change and also update documentation.

* Add `--db` option to analyze mode which creates database file
  containiing analysis result

* Add documentation on `rocpd` output format and analysis database file

* Create sqlite3 database using object relation mapping (ORM) provided
  by sqlalchemy library

* Fix metrics config to remove metrics marked as `null`, fix `Unit` header, add
  missing `title`

* Add test cases to ensure analysis data dump works

* Update CHANGELOG
@vedithal-amd

vedithal-amd commented Aug 15, 2025 •

Copy link
Copy Markdown
Contributor Author

As a general design, or user view, db output and df csv output are just alternatives to save the output as user's choice, they are just data storage, not analysis mode.

I would like to have the analysis database results calculation in a separate file like analysis_db.py instead of depending on analysis_cli.py.

analysis_cli.py only creates dataframe for metric values, it does not create dataframe for pc sampling and top kernels (instead it writes csv files, which is not necessary in analysis database mode). It also does not handle roofline data points calculation. That is why I am using different file analysis_db.py. This will require me to use a new analysis mode called analysis db mode. However, analysis mode is transparent to the end user, they don't specify analysis mode and should not care about it, all they have to do is pass --db option to analysis command.

@vedithal-amd

Copy link
Copy Markdown
Contributor Author

I will move roofline data point calculation logic inside another function in roofline_calc.py and use that in analysis_db.py

@vedithal-amd

Copy link
Copy Markdown
Contributor Author

Before removing the Grafana solution with Mongo DB. To reduce the confusion for the end users, we might want to explicitly claim there is no relationship for those 2 in the doc.

That is precisely why i am using analysis db or analysis data dump everywhere

@vedithal-amd

Copy link
Copy Markdown
Contributor Author

This PR will need dev efforts and re-testing due to #325. Calculation of roofline metrics should use the analysis config expressions, instead of hardcoded expressions.

pymongo
pyyaml
setuptools
sqlalchemy==2.0.42

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this need to be exactly this version? Can we do >= or <=? It is already not latest release

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea, will covert to >=, we don't want to be stuck using an outdated version

@vedithal-amd

Copy link
Copy Markdown
Contributor Author

Closing against #748

ammallya pushed a commit that referenced this pull request Jan 21, 2026
ammallya pushed a commit that referenced this pull request Apr 9, 2026
Creates a C API that wraps StatsClient and updates ais-stats to use the
new API
ammallya pushed a commit that referenced this pull request Apr 9, 2026
Creates a C API that wraps StatsClient and updates ais-stats to use the
new API

[ROCm/hipFile commit: 9e431f9]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants