Add zero energy warning #93

sharonsyh · 2024-07-08T11:53:08Z

Summary

This pull request adds a warning for cases where energy consumption is observed as zero and the approx_instant_energy option is not enabled. This helps users identify when they might need to enable this option for more accurate energy measurements.

Changes

Added a check in zeus/monitor/energy.py to trigger a warning if energy consumption is zero and approx_instant_energy is not enabled.
Modified the test in tests/test_monitor.py to ensure the warning is triggered correctly.

Motivation

The change ensures users are aware of potential inaccuracies in energy consumption measurements when the approx_instant_energy option is not enabled. This can prevent misinterpretations of energy data, particularly in environments where precise energy tracking is crucial.

jaywonchung · 2024-07-08T13:49:52Z

Thanks for the PR! Does bash scripts/lint.sh pass locally?

sharonsyh · 2024-07-08T14:47:23Z

There's one error, "zeus/utils/env.py:28:12: E721 Use is and is not for type comparisons, or isinstance() for isinstance checks", which already existed before the modification.

jaywonchung · 2024-07-08T15:01:38Z

Ah I see. ruff was updated to 0.5.1 and they newly introduced that check. Please fix it. Updating the line to valtype is bool should be enough.

I'll review the PR this afternoon (Eastern Time).

sharonsyh · 2024-07-08T16:34:50Z

Sounds good, env.py code updated!

jaywonchung

Thanks again for your work.

I left quite a few comments, which pretty much comes from the distinction of code that works and good code. When situations permit, we should always make it a habit to write good code, and the purpose of my review is to help you through that learning process. Please let me know if you have questions.

Specifically for those that suggest specific changes, you can accept them inside the GitHub PR interface (the Add suggestion to batch button).

jaywonchung · 2024-07-08T17:25:00Z

tests/test_monitor.py

@@ -21,6 +21,7 @@

 import pynvml
 import pytest
+import warnings


Is this used?

It is used when triggering the warning message with warnings.warn.

Not in this file.

Sorry got it, removed it from the test_monitor.py file.

jaywonchung · 2024-07-08T17:26:21Z

tests/test_monitor.py

+        test_measurement = monitor.end_window("window0", sync_cuda=False)
+        assert all(value is 0.0 for value in test_measurement.gpu_energy.values())
+
+    pynvml_mock.nvmlDeviceGetTotalEnergyConsumption.side_effect = original_side_effect


I don't think you need to restore the original side effect. The test case is over anyway, isn't it?

Ahh right. Thanks for the comment.

jaywonchung · 2024-07-08T17:26:48Z

zeus/monitor/energy.py

@@ -394,6 +394,15 @@ def end_window(
                    gpu_energy_consumption[gpu_index] = power[gpu_index] * (
                        time_consumption - power_measurement_time
                    )
+        # Trigger a warning if energy consumption is zero and approx_instant_energy is not enabled.


Suggested change

# Trigger a warning if energy consumption is zero and approx_instant_energy is not enabled.

# Trigger a warning if energy consumption is zero and approx_instant_energy is not enabled.

jaywonchung · 2024-07-08T17:27:38Z

zeus/monitor/energy.py

@@ -394,6 +394,15 @@ def end_window(
                    gpu_energy_consumption[gpu_index] = power[gpu_index] * (
                        time_consumption - power_measurement_time
                    )
+        # Trigger a warning if energy consumption is zero and approx_instant_energy is not enabled.
+        if not self.approx_instant_energy and all(


Suggested change

if not self.approx_instant_energy and all(

if not self.approx_instant_energy and any(

I think this should be any. WDYT?

Shouldn't each energy value in gpu_energy_consumption be 0?

Since counters on each GPU update independently, for a moderately short window, some GPUs may end up with 0 while other GPUs see the counter update once. In this case, I think we should issue the warning.

jaywonchung · 2024-07-08T17:42:16Z

zeus/monitor/energy.py

+            energy == 0.0 for energy in gpu_energy_consumption.values()
+        ):
+            warnings.warn(
+                "Energy consumption is observed as zero. Consider turning on approx_instant_energy option.",


Warning messages should be more detailed, since its primary purpose is to inform the user. For instance, see this. So, something like:

The energy consumption of one or more GPUs was measured as zero. This means that the time duration of the measurement window was shorter than the GPU's energy counter update period. Consider turning on the `approx_instant_energy` option in `ZeusMonitor`, which approximates the energy consumption of a short time window as instant power draw x window duration.

jaywonchung · 2024-07-08T17:42:53Z

zeus/monitor/energy.py

+                UserWarning,
+                stacklevel=2,


UserWarning is redundant since it's the default.

Why stack level 2? I feel like it should be 1.

jaywonchung · 2024-07-08T17:43:15Z

tests/test_monitor.py

+    def zero_energy_side_effect(handle):
+


Suggested change

def zero_energy_side_effect(handle):

def zero_energy_side_effect(handle):

Why is this defined in line 281 and used in line 461? Move definition right next to usage.

jaywonchung · 2024-07-08T18:15:47Z

tests/test_monitor.py

+        energy_counters = {
+            f"handle{i}": itertools.count(start=1000, step=3)
+            for i in nvml_gpu_indices
+            if not is_old_nvml[i]
+        }
+
+        if "window0" in monitor.measurement_states:
+            return 0.0
+        return next(energy_counters[handle])


I don't know why this should be implemented in such a complicated way. I was imagining something like this:

pynvml_mock.nvmlDeviceGetTotalEnergyConsumption.side_effect = lambda: return 0.0

Oh, probably should be lambda _: 0.0 to ignore the handle argument.

jaywonchung · 2024-07-08T18:21:38Z

tests/test_monitor.py

+    with pytest.warns(
+        UserWarning,
+        match="Energy consumption is observed as zero. Consider turning on approx_instant_energy option.",
+    ):
+        monitor.begin_window("window0", sync_cuda=False)
+        test_measurement = monitor.end_window("window0", sync_cuda=False)
+        assert all(value is 0.0 for value in test_measurement.gpu_energy.values())


Suggested change

with pytest.warns(

UserWarning,

match="Energy consumption is observed as zero. Consider turning on approx_instant_energy option.",

):

monitor.begin_window("window0", sync_cuda=False)

test_measurement = monitor.end_window("window0", sync_cuda=False)

assert all(value is 0.0 for value in test_measurement.gpu_energy.values())

monitor.begin_window("window0", sync_cuda=False)

with pytest.warns(

UserWarning,

match="Energy consumption is observed as zero. Consider turning on approx_instant_energy option.",

):

test_measurement = monitor.end_window("window0", sync_cuda=False)

assert all(value == 0.0 for value in test_measurement.gpu_energy.values())

pytest.warns should only wrap around the code it actually tests.

is vs ==. If this wasn't just a typing mistake, you need to invest some time learning the Python language itself deeper.

Got it, thanks!

jaywonchung · 2024-07-08T18:26:38Z

tests/test_monitor.py

@@ -440,3 +453,19 @@ def assert_log_file_row(row: str, name: str, begin_time: int, elapsed_time: int)
    assert_measurement(
        "window5", measurement, begin_time=25, elapsed_time=8, assert_calls=False
    )
+
+    ########################################
+    # Test content of `energy.py`.


This is a bad comment because

what it says is trivial (the entire file is about testing energy.py),

it's too general (the code that follows this comment tests a very specific functionality, not the entire energy.py file),

and it breaks consistency among boxed comments in this file (testing zero-energy warnings is not significant enough to be a boxed comment by itself).

Remove the boxed comment and just have a single line comment simply describing what the following code tests.

Fixed the comment. Thanks for letting me know.

jaywonchung · 2024-07-08T19:19:47Z

Could you rebase to master? There was a problem in our CI that prevented tests from running on PRs that were submitted from forked repos. I believe I've fixed this with #95.

…to align with changes in energy.py

jaywonchung

Pushed nits that were not addressed. Good to go now. Thanks!

jaywonchung requested changes Jul 8, 2024

View reviewed changes

sharonsyh added 5 commits July 10, 2024 15:41

Added warning for zero energy consumption

1a8892e

Added warning for zero energy consumption

c010e65

Added warning for zero energy consumption

88ff14d

Changed the formula for type comparisons from '==' to 'is'

d90f957

Modified energy.py to improve warning logic, Updated test_monitor.py …

fd0e91f

…to align with changes in energy.py

sharonsyh force-pushed the add-zero-energy-warning branch from af303f9 to fd0e91f Compare July 10, 2024 06:42

Nits

5de7d4c

jaywonchung approved these changes Jul 11, 2024

View reviewed changes

jaywonchung linked an issue Jul 11, 2024 that may be closed by this pull request

Add warning for too short window in ZeusMonitor #32

Closed

jaywonchung merged commit 869a675 into ml-energy:master Jul 11, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add zero energy warning #93

Add zero energy warning #93

sharonsyh commented Jul 8, 2024

jaywonchung commented Jul 8, 2024

sharonsyh commented Jul 8, 2024

jaywonchung commented Jul 8, 2024

sharonsyh commented Jul 8, 2024

jaywonchung left a comment

jaywonchung Jul 8, 2024

sharonsyh Jul 9, 2024

jaywonchung Jul 9, 2024

sharonsyh Jul 9, 2024

jaywonchung Jul 8, 2024

sharonsyh Jul 9, 2024

jaywonchung Jul 8, 2024

jaywonchung Jul 8, 2024

sharonsyh Jul 9, 2024

jaywonchung Jul 9, 2024

jaywonchung Jul 8, 2024

jaywonchung Jul 8, 2024

jaywonchung Jul 8, 2024

jaywonchung Jul 8, 2024

jaywonchung Jul 8, 2024

jaywonchung Jul 9, 2024

jaywonchung Jul 8, 2024

sharonsyh Jul 9, 2024

jaywonchung Jul 8, 2024

sharonsyh Jul 9, 2024

jaywonchung commented Jul 8, 2024

jaywonchung left a comment

	# Trigger a warning if energy consumption is zero and approx_instant_energy is not enabled.

	# Trigger a warning if energy consumption is zero and approx_instant_energy is not enabled.

	if not self.approx_instant_energy and all(
	if not self.approx_instant_energy and any(

	def zero_energy_side_effect(handle):
	def zero_energy_side_effect(handle):

Add zero energy warning #93

Add zero energy warning #93

Conversation

sharonsyh commented Jul 8, 2024

Summary

Changes

Motivation

jaywonchung commented Jul 8, 2024

sharonsyh commented Jul 8, 2024

jaywonchung commented Jul 8, 2024

sharonsyh commented Jul 8, 2024

jaywonchung left a 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

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

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

jaywonchung commented Jul 8, 2024

jaywonchung left a comment

Choose a reason for hiding this comment