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.

Sign up for GitHub

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

Modify p4studio interactive to use limited parallel jobs #78

Merged
merged 16 commits into from
Feb 20, 2025
Merged

Modify p4studio interactive to use limited parallel jobs #78

Changes from 15 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
39d35f3
Enable batch-install.sh to run on Ubuntu 22.04
jafingerhut Jan 16, 2025
ecc22c1
Merge remote-tracking branch 'upstream/main' into main
jafingerhut Jan 16, 2025
5f5c312
Merge branch 'main' of https://github.com/p4lang/open-p4studio into main
jafingerhut Jan 17, 2025
113e4f1
Merge branch 'main' of https://github.com/p4lang/open-p4studio
jafingerhut Jan 17, 2025
4becb30
Merge branch 'main' of github.com:jafingerhut/open-p4studio
jafingerhut Jan 17, 2025
d94cdd5
Merge branch 'main' of https://github.com/p4lang/open-p4studio
jafingerhut Jan 21, 2025
5a28bf0
Merge branch 'main' of https://github.com/p4lang/open-p4studio
jafingerhut Jan 21, 2025
14807ca
Merge remote-tracking branch 'upstream/main'
jafingerhut Jan 22, 2025
38a5340
Merge remote-tracking branch 'upstream/main' into main
jafingerhut Jan 29, 2025
09b4a9c
Merge branch 'main' of https://github.com/p4lang/open-p4studio into main
jafingerhut Jan 29, 2025
2f3cf06
Merge branch 'main' of https://github.com/p4lang/open-p4studio into main
jafingerhut Feb 5, 2025
f7ea382
Merge remote-tracking branch 'up/main'
jafingerhut Feb 17, 2025
7533bd6
Merge branch 'main' of https://github.com/p4lang/open-p4studio into main
jafingerhut Feb 18, 2025
ce6ab36
Merge branch 'main' of https://github.com/p4lang/open-p4studio into main
jafingerhut Feb 19, 2025
4798fe8
Fix #77 Limit parallel jobs used during command 'p4studio interactive'
jafingerhut Feb 19, 2025
dfabc14
Force CI to use `nproc` parallel jobs
jafingerhut Feb 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions p4studio/interactive/interactive_command.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
from config.configuration_manager import current_configuration_manager
from interactive.input_validation import validate_path_to_write, validate_kernel_headers_input, validate_file_to_read
from profile.profile import Profile
from profile.profile_command import execute_plan
from profile.profile_command import execute_plan, calculate_jobs_from_available_cpus_and_memory
from profile.profile_execution_plan import ProfileExecutionPlan
from system.check_system_utils import print_multiple_checks
from system.checks import get_initial_checks
Expand Down Expand Up @@ -166,7 +166,8 @@ def create_default_profile() -> Profile:


def build_sde(context: Context, profile: Profile) -> None:
plan = ProfileExecutionPlan(profile, None, os.cpu_count())
jobs = calculate_jobs_from_available_cpus_and_memory()
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably should be an argument from the previous calculation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am sorry, but I do not understand the comment. The new first line of build_sde calculates and stores a value in variable jobs. The second line calls ProfileExecutionPlan, with the 3rd parameter being jobs instead of os.cpu_count().

What are you recommending I change?

Copy link
Contributor

Choose a reason for hiding this comment

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

open-p4studio/p4studio/profile/profile_command.py

Line 215 in c5c4e98

jobs = calculate_jobs_from_available_cpus_and_memory()

this calculation is already done here, no? Ideally, we should only do this once and then reuse the result in other contexts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case it is not obvious: the code in this file is run if you do a command p4studio interactive, which is what the install.sh bash script invokes. If you do this, the other Python source file that has the definition of calculate_jobs_from_available_cpus_and_memory has none of its code executed at all, unless it is explicitly called from somewhere in this file. That is what this change is adding -- an explicit call from this file into the function defined in the other file, that calculates a reasonable number of parallel jobs to use.

This is not a repeated call of that function in this code path. It is the only one.

Copy link
Contributor

@fruffy fruffy Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What is the common ancestor between the profile and the interactive command? I was thinking that this ancestor should call the function, then pass the result to either one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the common ancestor is some click module Python code that parses some command line arguments, and decides via click annotations on Python methods which function to invoke. If you do p4studio profile <some args> on the command line, it invokes a method in the file profile_command.py. If you do p4studio interactive, it iknvokes a method in this file being modified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note this line early in this file:

from profile.profile_command import execute_plan, calculate_jobs_from_available_cpus_and_memory

There is an execute_plan function called from both profile_command.py and interactive_command.py that is doing most of the work of building and installing the software. Whoever wrote this code decided to define it in profile_command.py and call it from here, rather than putting execute_plan in some common 3rd file and calling it from both places. I'm not adding new code flows that didn't exist between files already here. I'm adding one more cross-file call where there was already a more significant one before.

Copy link
Contributor

@fruffy fruffy Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, I think we can leave it there then. It seems like checks like these should be set from both workflows, but it may be that the interactive command has less restrictions and doesn't even invoke system checks or common code.

plan = ProfileExecutionPlan(profile, None, jobs)
execute_plan(context, plan)


Expand Down
Loading