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

chore: load cpu architecture dependent models #1800

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rootfs
Copy link
Contributor

@rootfs rootfs commented Sep 26, 2024

This is to prepare for coming CPU dependent models

@rootfs rootfs requested review from sthaha and sunya-ch September 26, 2024 14:53
Copy link
Contributor

github-actions bot commented Sep 26, 2024

🤖 SeineSailor

Here is a concise summary of the pull request changes:

Summary: This pull request, "chore: load cpu architecture dependent models", enables loading of CPU architecture-dependent models by introducing a cpuArch parameter and incorporating node.CPUArchitecture() in various functions. The changes prepare the codebase for CPU-dependent models, affecting power model loading behavior based on CPU architecture.

Key Modifications:

  1. Added cpuArch parameter to GetDefaultPowerModelURL function.
  2. Incorporated node.CPUArchitecture() in multiple functions.
  3. Modified GetNodePlatformPowerFromDummyServer, GetNodeComponentsPowerFromDummyServer, and GetProcessComponentsPower functions to consider CPU architecture.
  4. Updated genRegressor function to load CPU architecture-specific models.

Impact: These changes will allow the codebase to load power models based on CPU architecture, potentially affecting the behavior of the code when loading power models.

Observations/Suggestions:

  • It would be beneficial to include additional tests to ensure the correct loading of CPU architecture-dependent models.
  • Consider documenting the implications of CPU architecture on power model loading behavior for future reference.
  • Review the modified functions to ensure they handle different CPU architectures correctly.

@rootfs rootfs requested a review from KaiyiLiu1234 September 26, 2024 15:02
cpuArch = strings.TrimSpace(cpuArch)
fullPath := fmt.Sprintf(`/var/lib/kepler/data/%s/model_weight/%s_%sModel.json`, cpuArch, energySource, modelOutputType)
// if the model does not exist, return the default model
if _, err := os.Stat(fullPath); os.IsNotExist(err) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this result in a possible race condition? If you can guarantee that after the os.Stat the file exists, then ignore this comment.

return fmt.Sprintf(`/var/lib/kepler/data/model_weight/%s_%sModel.json`, energySource, modelOutputType)
func GetDefaultPowerModelURL(modelOutputType, energySource, cpuArch string) string {
// strip white space or new line from cpuArch
cpuArch = strings.TrimSuffix(cpuArch, "\n")
Copy link
Collaborator

Choose a reason for hiding this comment

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

TrimSpace should remove \n characters and white space, so you can remove this line

cpuArch = strings.TrimSpace(cpuArch)
fullPath := fmt.Sprintf(`/var/lib/kepler/data/%s/model_weight/%s_%sModel.json`, cpuArch, energySource, modelOutputType)
// if the model does not exist, return the default model
if _, err := os.Stat(fullPath); os.IsNotExist(err) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

On a minor note, os.isNotExist(err) might be misleading as os.Stat can return other types of errors which is not the same as a file/model not existing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

these files shouldn't change at all 🤔

Copy link
Collaborator

@sthaha sthaha left a comment

Choose a reason for hiding this comment

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

LGTM! but requesting change to remove the ebpf file changes.

@rootfs rootfs marked this pull request as draft November 1, 2024 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants