Skip to content

Conversation

granulatedekel
Copy link

@granulatedekel granulatedekel commented Sep 23, 2024

Part of the G-UTILS-GPROFILER REWORK saga - which aims to move away shared code from gprofiler to the g-utils project:
intel/granulate-utils#263 intel/granulate-utils#261 #925 #926 #926

This is the ultimate PR which is responsible for handling the last few files that required logical changes as well

@granulatedekel granulatedekel changed the title rework complex files to use granulate-utils G-UTILS REWORK (3): Rework complex files Sep 23, 2024
@granulatedekel granulatedekel changed the title G-UTILS REWORK (3): Rework complex files G-UTILS-GPROFILER REWORK (5): Rework complex files Sep 23, 2024
@granulatedekel granulatedekel added the g-utils-gprofiler-rework PRs related to the G-UTILS-GPROFILER saga label Sep 23, 2024
@granulatedekel granulatedekel marked this pull request as ready for review September 23, 2024 22:52
@roi-granulate
Copy link
Contributor

roi-granulate commented Oct 1, 2024

@granulatedekel I’m getting into this here
but before I dig into the code - shouldn’t this PR CI be ultimately green?
because PR no.5 should point to granulate-utils’ PR no. 2, which, as I understand, should represent the full code merged into both repo’s main branches (== the final product)
what am I missing?

@YoniKF
Copy link
Contributor

YoniKF commented Oct 1, 2024

@roi-granulate

@granulatedekel I’m getting into this here but before I dig into the code - shouldn’t this PR CI be ultimately green? because PR no.5 should point to granulate-utils’ PR no. 2, which, as I understand, should represent the full code merged into both repo’s main branches (== the final product) what am I missing?

Linter failure:

ERROR: /home/runner/work/gprofiler/gprofiler/gprofiler/utils/__init__.py Imports are incorrectly sorted and/or formatted.
Skipped 2 files
ERROR: /home/runner/work/gprofiler/gprofiler/gprofiler/profilers/java.py Imports are incorrectly sorted and/or formatted.

Probably due to bulk rename of modules without reordering. @granulatedekel will fix but I don't think that this blocks review.

@YoniKF YoniKF requested a review from roi-granulate October 1, 2024 10:16
@roi-granulate
Copy link
Contributor

@YoniKF indeed minor fix - but it blocks tests from running, so we have no indication on breakages.
I will start reviewing, but please fix this.

from gprofiler.consts import CPU_PROFILING_MODE
from gprofiler.platform import is_linux, is_windows
from granulate_utils.gprofiler.platform import is_windows
import granulate_utils.gprofiler.utils as _utils
Copy link
Contributor

@Jongy Jongy Oct 8, 2024

Choose a reason for hiding this comment

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

Note that moving run_process to granulate-utils poses a problem, since in another PR we're adding more gprofiler-specific behavior to that function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
g-utils-gprofiler-rework PRs related to the G-UTILS-GPROFILER saga
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants