Skip to content
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

G-UTILS-GPROFILER REWORK (5): Rework complex files #928

Open
wants to merge 2 commits into
base: refactor-into-g-utils-condensed
Choose a base branch
from

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:
Granulate/granulate-utils#263 Granulate/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
Collaborator

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.

@roi-granulate
Copy link
Collaborator

@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