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

Clean up flag formatting in PKB #5241

Open
jellyfishcake opened this issue Oct 4, 2024 · 5 comments
Open

Clean up flag formatting in PKB #5241

jellyfishcake opened this issue Oct 4, 2024 · 5 comments
Labels
good first issue This is a good issue for someone new to PKB

Comments

@jellyfishcake
Copy link
Contributor

flags should take the form:
FOOBAR = flags.DEFINE...
https://github.com/GoogleCloudPlatform/PerfKitBenchmarker/blob/master/perfkitbenchmarker/linux_benchmarks/sysbench_benchmark.py#L135

Reference the flag later with:
x = FOOBAR.value
https://github.com/GoogleCloudPlatform/PerfKitBenchmarker/blob/master/perfkitbenchmarker/linux_benchmarks/sysbench_benchmark.py#L810

@jellyfishcake jellyfishcake added the good first issue This is a good issue for someone new to PKB label Oct 4, 2024
@ronaksingh27
Copy link
Contributor

@jellyfishcake I have reviewed the flag definition and usage, and it looks like the code is already following the required format. The flag _SCALE_UP_MAX_CPU_UTILIZATION is properly defined using flags.DEFINE_float and is referenced with .value later in the code.

If there are any other flags that need updating or if I missed something, please let me know, and I’ll be happy to make the necessary adjustments!

Thanks again for your input, and I appreciate your guidance! 😊

@jellyfishcake
Copy link
Contributor Author

There are a lot of flags scattered all over the code base. One example is this: https://github.com/GoogleCloudPlatform/PerfKitBenchmarker/blob/master/perfkitbenchmarker/linux_benchmarks/cluster_boot_benchmark.py#L124

@wlifferth
Copy link
Contributor

wlifferth commented Oct 4, 2024

Because we have so many flags and the process for reformatting is straightforward this is a great starter issue that multiple people could work on. Here's a basic guide to get started:

  1. Find a file that still has the old flag style
    • You can use this search query to find files with flags, and look for instances of the old style, where flags are defined like flags.DEFINE_{data_type}(... (without assigning the flag to a variable) and referenced like flags.flag_name
    • Many files have some flags using the old style, and some flags using the new style (the new style looks like FLAG_NAME = flags.DEFINE_{data_type}(... and x = FLAG_NAME.value)--thats okay! We'll just want to update the flags that still use the old style.
  2. File an issue with the name Updating flag formatting in {FILE_NAME}
    • this will let other contributors know you're working on reformatting the flags in this file, so we don't have multiple people working on the same flags
    • cross link it with this issue, and make sure the issue is assigned to you
  3. Update the flags in the file you choose
    • To keep things simple, we recommend you only update flags that only appear in a single file. For example, if we search for all instances of flags.cluster_boot_time_reboot we can see it only shows up in perfkitbenchmarker/linux_benchmarks/cluster_boot_benchmark.py which makes it a good candidate for a starter pull request.
    • For example, if our flag was called vm_count the old code might look like:
      • definition: flags.DEFINE_integer('vm_count',...
      • usage: vm_count = flags.vm_count
        and we want to update it to look like:
      • definition: VM_COUNT = flags.DEFINE_integer('vm_count',...
      • usage: vm_count = VM_COUNT.value
  4. Create a pull request, then find another file and repeat if you want to!

Comment here if you have any questions!

@ronaksingh27
Copy link
Contributor

@wlifferth Thank you for the detailed guide! The clear steps and examples make it easy to follow, especially for a beginner like me. I’ll get started on updating the flags and follow the process you outlined. Appreciate the support and help!

@ronaksingh27
Copy link
Contributor

@jellyfishcake I did as said , had some CLA issue , got it resolved , could you please review and give feedbacks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue This is a good issue for someone new to PKB
Projects
None yet
Development

No branches or pull requests

3 participants