[mstx] fix: skip mstx_preprocessing if necessary#31
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where the MSTX preprocessing step would run unnecessarily even if the output had already been generated. It introduces a check for existing parsed output, allowing the process to be skipped, thereby improving efficiency. Additionally, it refines the example execution script for better clarity and consistency in variable usage. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the mstx_exec.sh script to use MSTX_PROFILER_DATA_PATH instead of TORCH_PROFILER_DATA_PATH for input paths and renames the profiler analysis title. In mstx_preprocessing.py, it refactors logging to use loguru, removes top-level torch_npu imports, and adds logic to skip re-analysis if parsed output already exists, with torch_npu components now lazily imported. Feedback includes correcting a critical typo (MSXT_PROFILER_DATA_PATH to MSTX_PROFILER_DATA_PATH) in the shell script and ensuring the variable is properly defined. Additionally, an improvement was suggested to simplify the conditional check for lazy imports in mstx_preprocessing.py.
| echo "MSTX Profiler Cluster Analysis" | ||
| echo "==========================================" | ||
| echo "Input Path: ${TORCH_PROFILER_DATA_PATH}" | ||
| echo "Input Path: ${MSXT_PROFILER_DATA_PATH}" |
There was a problem hiding this comment.
There appears to be a typo in the variable name. It should be MSTX_PROFILER_DATA_PATH instead of MSXT_PROFILER_DATA_PATH. This typo is also present on lines 25 and 30.
Additionally, this new variable is used but not defined within the script. The original variable TORCH_PROFILER_DATA_PATH was initialized on line 5. You might want to replace that initialization with one for MSTX_PROFILER_DATA_PATH to avoid potential errors if the environment variable is not set.
| echo "Input Path: ${MSXT_PROFILER_DATA_PATH}" | |
| echo "Input Path: ${MSTX_PROFILER_DATA_PATH}" |
| echo ">>> Start mstx data preprocessing..." | ||
|
|
||
| python -m rl_insight.utils.mstx_preprocessing "${TORCH_PROFILER_DATA_PATH}" | ||
| python -m rl_insight.utils.mstx_preprocessing "${MSXT_PROFILER_DATA_PATH}" |
There was a problem hiding this comment.
Correcting the typo in the variable name: MSXT_PROFILER_DATA_PATH -> MSTX_PROFILER_DATA_PATH.
| python -m rl_insight.utils.mstx_preprocessing "${MSXT_PROFILER_DATA_PATH}" | |
| python -m rl_insight.utils.mstx_preprocessing "${MSTX_PROFILER_DATA_PATH}" |
|
|
||
| python -m rl_insight.main \ | ||
| --input-path "${TORCH_PROFILER_DATA_PATH}" \ | ||
| --input-path "${MSXT_PROFILER_DATA_PATH}" \ |
There was a problem hiding this comment.
Correcting the typo in the variable name: MSXT_PROFILER_DATA_PATH -> MSTX_PROFILER_DATA_PATH.
| --input-path "${MSXT_PROFILER_DATA_PATH}" \ | |
| --input-path "${MSTX_PROFILER_DATA_PATH}" \ |
| continue | ||
|
|
||
| try: | ||
| if analyse is None or export_type is None: |
There was a problem hiding this comment.
Since analyse and export_type are always initialized together inside this block, you only need to check for one of them to decide whether to perform the lazy import. Checking for analyse is None is sufficient.
| if analyse is None or export_type is None: | |
| if analyse is None: |
1 participant
What does this PR do?
skip mstx_preprocessing if necessary
issue -> #20
Checklist Before Starting
[{modules}] {type}: {description}(This will be checked by the CI){modules}includemstx,mvtx,torch_profile,deployment,perf,algo,env,doc,data,cfg,ci,misc,,like[mstx, ci]{type}is infeat,fix,refactor,chore,test[BREAKING]to the beginning of the title.[BREAKING][mstx, torch_profile] feat: support timeline parsingTest
API and Usage Example
# Add code snippet or script demonstrating how to use thisDesign & Code Changes
Checklist Before Submitting
Important
Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.
pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=always