fix platform power return unit #1468

sunya-ch · 2024-05-23T14:30:36Z

This PR is to fix the issue regarding platform power reported in the slack channel as well as my direct experience in #1466.

I have found the different handling between component power and platform power in the model package.
Power model return power in joule unit in float. However, we handle the power in millijoule in Kepler.
Component power has been converted correctly by GetComponentPower in utils module. On the other hand, platform power is used as-is.

After fix:

As seen in the above validation, there are still more points to investigate or fix.
I have stressed 70% of CPU in a single CPU (it supposes to use 700ms).
The most critical part is the BPF CPU time. As seen above, the total CPU time is much higher than stress time.
However, as seen in the top two panel which are the power meters, the power is significantly increased at the stress time.

Another point need concern is the model that are being used. The default one is 4 cores which produces maximum power at around 200W. In the above result, I changed to use 32 cores model. We need to change default power model to the largest one on the estimator case.

Signed-off-by: Sunyanan Choochotkaew sunyanan.choochotkaew1@ibm.com

github-actions · 2024-05-23T14:31:16Z

🤖 SeineSailor

Here is a concise summary of the pull request changes:

Summary: This pull request addresses issues related to platform power handling in the Kepler project, focusing on consistent energy unit handling and correction of power estimation functions.

Key Modifications:

Logging and unit conversion: Added logging statements for idle and dynamic energy stats, and corrected the conversion of energy units from millijoules to joules.
Constant renaming: Renamed MiliJouleToJoule to MilliJouleToJoule to maintain consistency.
Power estimation function updates: Modified GetPlatformPower functions to return platform power in millijoules instead of joules, and updated return types from []float64 to []uint64.
New utility function: Introduced utils.GetPlatformPower to convert power values from floats to uint64.

Impact: These changes ensure consistent energy unit handling across components and platforms, aligning with the rest of the Kepler project. The updates to power estimation functions and the introduction of a new utility function improve the accuracy and reliability of power handling in the codebase.

Observations/Suggestions:

It would be beneficial to include additional tests to verify the correctness of the updated power estimation functions and ensure that the changes do not introduce any regressions.
Consider adding documentation or comments to explain the reasoning behind the changes and the importance of consistent energy unit handling in the Kepler project.
The pull request could be further improved by breaking down the changes into smaller, more focused commits, making it easier to review and understand the individual modifications.

dave-tucker

A couple of nits, but overall LGTM

dave-tucker · 2024-05-23T14:40:57Z

pkg/model/estimator/local/regressor/exponential_test.go

@@ -31,7 +31,7 @@ var (
 var _ = Describe("Test Exponential Predictor Unit", func() {
 	It("Get Node Platform Power By Exponential Regression", func() {
 		powers := GetNodePlatformPowerFromDummyServer(dummyExponentialWeightHandler, types.ExponentialTrainer)
-		Expect(int(powers[0])).Should(BeEquivalentTo(4))
+		Expect(int(powers[0]/1000) * 1000).Should(BeEquivalentTo(4000))


Doesn't the *1000 and /1000 cancel out?

These are to ignore the leftover from exponential, logarithm, and logistic function but still keep the unit to millijoule.

It may make sense to break this out into a test helper function with a comment on what it's doing then.

dave-tucker · 2024-05-23T14:41:06Z

pkg/model/estimator/local/regressor/logarithm_test.go

@@ -31,7 +31,7 @@ var (
 var _ = Describe("Test Logarithmic Predictor Unit", func() {
 	It("Get Node Platform Power By Logarithmic Regression", func() {
 		powers := GetNodePlatformPowerFromDummyServer(dummyLogarithmicWeightHandler, types.LogarithmicTrainer)
-		Expect(int(powers[0])).Should(BeEquivalentTo(2))
+		Expect(int(powers[0]/1000) * 1000).Should(BeEquivalentTo(2000))


dave-tucker · 2024-05-23T14:41:12Z

pkg/model/estimator/local/regressor/logistic_test.go

@@ -31,7 +31,7 @@ var (
 var _ = Describe("Test Logistic Predictor Unit", func() {
 	It("Get Node Platform Power By Logistic Regression", func() {
 		powers := GetNodePlatformPowerFromDummyServer(dummyLogisticWeightHandler, types.LogisticTrainer)
-		Expect(int(powers[0])).Should(BeEquivalentTo(2))
+		Expect(int(powers[0]/1000) * 1000).Should(BeEquivalentTo(2000))


pkg/collector/stats/stats.go

dave-tucker · 2024-05-23T15:32:51Z

pkg/metrics/consts/conts.go

@@ -24,7 +24,7 @@ const (
 	MetricsNamespace       = "kepler"
 	EnergyMetricNameSuffix = "_joules_total"
 	UsageMetricNameSuffix  = "_total"
-	MiliJouleToJoule       = 1000
+	MilliJouleToJoule      = 1000


nit: This could be used to convert in either direction depending on the operator used. So it may be better to rename something like JouleMillijouleConversionFactor 🤷 naming is hard.

dave-tucker · 2024-05-23T16:34:05Z

pkg/model/estimator/local/ratio.go

@@ -104,7 +104,7 @@ func (r *RatioPowerModel) GetPlatformPower(isIdlePower bool) ([]float64, error)
 		} else {
 			processPower = r.getPowerByRatio(processIdx, int(PlatformUsageMetric), int(PlatformDynPower), numProcesses)


It may make more sense here to fix the return value of r.getPowerByRatio given the float is immediately converted to uint64 here.

dave-tucker · 2024-05-23T16:43:37Z

pkg/model/estimator/local/regressor/exponential_test.go

@@ -31,7 +31,7 @@ var (
 var _ = Describe("Test Exponential Predictor Unit", func() {
 	It("Get Node Platform Power By Exponential Regression", func() {
 		powers := GetNodePlatformPowerFromDummyServer(dummyExponentialWeightHandler, types.ExponentialTrainer)
-		Expect(int(powers[0])).Should(BeEquivalentTo(4))
+		Expect(int(powers[0]/1000) * 1000).Should(BeEquivalentTo(4000))


It may make sense to break this out into a test helper function with a comment on what it's doing then.

dave-tucker · 2024-05-23T16:45:28Z

pkg/model/utils/utils.go

@@ -21,7 +21,7 @@ import (
 )

 const (
-	jouleToMiliJoule = 1000
+	jouleToMilliJoule = 1000


Can this use the same constant as eariler?

sunya-ch · 2024-05-24T00:34:23Z

@dave-tucker Thank you so much for the review.
I pushed the changes regarding your review. Please confirm here.

[edit] add minor go-lint fix: here

Signed-off-by: Sunyanan Choochotkaew <sunyanan.choochotkaew1@ibm.com>

dave-tucker

LGTM!

sunya-ch force-pushed the fix-bugs branch from 636a6ec to 6184c87 Compare May 23, 2024 14:42

dave-tucker reviewed May 23, 2024

View reviewed changes

sunya-ch force-pushed the fix-bugs branch from 6184c87 to b0ffdd2 Compare May 23, 2024 14:49

dave-tucker reviewed May 23, 2024

View reviewed changes

sunya-ch force-pushed the fix-bugs branch from b0ffdd2 to 1de451f Compare May 24, 2024 00:31

fix platform power return unit

3d52e97

Signed-off-by: Sunyanan Choochotkaew <sunyanan.choochotkaew1@ibm.com>

sunya-ch force-pushed the fix-bugs branch from 1de451f to 3d52e97 Compare May 24, 2024 00:36

dave-tucker approved these changes May 24, 2024

View reviewed changes

rootfs merged commit ada7884 into sustainable-computing-io:main May 24, 2024
22 checks passed

This was referenced May 30, 2024

bpf: fix the process parameter order to match the c and go code #1479

Merged

Fix bug in the ebpf metric collection #1481

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix platform power return unit #1468

fix platform power return unit #1468

sunya-ch commented May 23, 2024 •

edited

Loading

github-actions bot commented May 23, 2024 •

edited

Loading

dave-tucker left a comment

dave-tucker May 23, 2024

sunya-ch May 23, 2024 •

edited

Loading

dave-tucker May 23, 2024

dave-tucker May 23, 2024

dave-tucker May 23, 2024

dave-tucker May 23, 2024

dave-tucker May 23, 2024

dave-tucker May 23, 2024

dave-tucker May 23, 2024

sunya-ch commented May 24, 2024 •

edited

Loading

dave-tucker left a comment

		@@ -104,7 +104,7 @@ func (r *RatioPowerModel) GetPlatformPower(isIdlePower bool) ([]float64, error)
		} else {
		processPower = r.getPowerByRatio(processIdx, int(PlatformUsageMetric), int(PlatformDynPower), numProcesses)

fix platform power return unit #1468

fix platform power return unit #1468

Conversation

sunya-ch commented May 23, 2024 • edited Loading

github-actions bot commented May 23, 2024 • edited Loading

dave-tucker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunya-ch May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunya-ch commented May 24, 2024 • edited Loading

dave-tucker left a comment

Choose a reason for hiding this comment

sunya-ch commented May 23, 2024 •

edited

Loading

github-actions bot commented May 23, 2024 •

edited

Loading

sunya-ch May 23, 2024 •

edited

Loading

sunya-ch commented May 24, 2024 •

edited

Loading