Adding Billing Rates to the config.py and rewriting references #159

jimmysway · 2025-10-09T14:36:34Z

Part 2 of #144

QuanMPhm · 2025-10-16T17:48:35Z

openshift_metrics/config.py

+RATE_GPU_H100_SU = os.getenv("RATE_GPU_H100_SU", "6.04")
+
+# Legacy rate (for backward compatibility)
+GPU_A100_RATE = os.getenv("GPU_A100_RATE", "1.803")


Where is this constant used?

https://github.com/CCI-MOC/openshift-usage-scripts/blob/main/openshift_metrics/invoice.py

Lines 230

I couldn't find it. A text search across your PR doesn't show any reference to GPU_A100_RATE

QuanMPhm · 2025-10-16T17:54:02Z

openshift_metrics/config.py

+SU_NAMES = ["GPUV100", "GPUA100", "GPUA100SXM4", "GPUH100", "CPU"]
+RESOURCE_NAMES = ["vCPUs", "RAM", "GPUs"]


These are not configurations for the script. I would suggest leave them where they were in get_su_definitions for now. As mentioned by @naved001, we could move this information to a file at some point in the future.

Yes, exactly. Let's not touch these now for now. The whole SU types and their names will require some more thought.

@jimmysway This comment is not addressed. These constants are not accessed anywhere in your PR

QuanMPhm · 2025-10-16T17:55:16Z

openshift_metrics/merge.py

+    parser.add_argument("--rate-cpu-su", type=Decimal, default=Decimal(RATE_CPU_SU))
+    parser.add_argument(
+        "--rate-gpu-v100-su", type=Decimal, default=Decimal(RATE_GPU_V100_SU)
+    )
+    parser.add_argument(
+        "--rate-gpu-a100sxm4-su", type=Decimal, default=Decimal(RATE_GPU_A100SXM4_SU)
+    )
+    parser.add_argument(
+        "--rate-gpu-a100-su", type=Decimal, default=Decimal(RATE_GPU_A100_SU)
+    )
+    parser.add_argument(
+        "--rate-gpu-h100-su", type=Decimal, default=Decimal(RATE_GPU_H100_SU)
+    )


If the point of this PR is to have configuration done through env vars, I would suggest removing these parser arguments since they are now redundant.

QuanMPhm · 2025-10-16T18:00:56Z

openshift_metrics/config.py

+RATE_CPU_SU = os.getenv("RATE_CPU_SU", "0.013")
+RATE_GPU_V100_SU = os.getenv("RATE_GPU_V100_SU", "1.214")
+RATE_GPU_A100SXM4_SU = os.getenv("RATE_GPU_A100SXM4_SU", "2.078")
+RATE_GPU_A100_SU = os.getenv("RATE_GPU_A100_SU", "1.803")
+RATE_GPU_H100_SU = os.getenv("RATE_GPU_H100_SU", "6.04")


@naved001 Do we still want keep the old behavior of having the rates None if not provided?

Yeah, good point. If someone were to forget setting I'd rather the code fail than use some default values for rates.

naved001 · 2025-11-10T21:45:58Z

@jimmysway have you had a chance to address the comments in this PR?

…and rewriting code references. Removed CLI arguments for setting rates

…ng-config

jimmysway · 2025-11-11T15:32:55Z

Sorry I had them sitting in my local branch. I just merged with upstream

QuanMPhm · 2025-11-18T14:57:55Z

@jimmysway There are merge conflicts in the PR

QuanMPhm

A few previous comments were not addressed. I've also added a few new ones

QuanMPhm · 2025-12-16T16:01:12Z

openshift_metrics/config.py

+SU_NAMES = ["GPUV100", "GPUA100", "GPUA100SXM4", "GPUH100", "CPU"]
+RESOURCE_NAMES = ["vCPUs", "RAM", "GPUs"]


@jimmysway This comment is not addressed. These constants are not accessed anywhere in your PR

QuanMPhm · 2025-12-16T16:06:39Z

openshift_metrics/config.py

+USE_NERC_RATES_ENV = os.getenv("USE_NERC_RATES")
+USE_NERC_RATES = USE_NERC_RATES_ENV.lower() == "true" if USE_NERC_RATES_ENV else None


I would suggest just having USE_NERC_RATES and removing USE_NERC_RATES_ENV. If the user does not set USE_NERC_RATES, that should be equivalent to setting the env var to false:

Suggested change

USE_NERC_RATES_ENV = os.getenv("USE_NERC_RATES")

USE_NERC_RATES = USE_NERC_RATES_ENV.lower() == "true" if USE_NERC_RATES_ENV else None

USE_NERC_RATES = os.getenv("USE_NERC_RATES", "false").lower() == "true"

Do you have a specific reason for why these two env vars needs to be defined seperately?

QuanMPhm · 2025-12-16T16:09:55Z

openshift_metrics/config.py

+RATE_GPU_H100_SU = os.getenv("RATE_GPU_H100_SU")
+
+# Legacy rate (for backward compatibility)
+GPU_A100_RATE = os.getenv("GPU_A100_RATE")


I still don't see where this constant is used

QuanMPhm · 2025-12-16T16:12:16Z

openshift_metrics/merge.py

+    if USE_NERC_RATES is None:
+        raise ValueError(
+            "USE_NERC_RATES environment variable must be set to 'true' or 'false'"
+        )


Following from my comment above, this if clause should also be removed. I think USE_NERC_RATES not being set should be interpreted as False

QuanMPhm · 2025-12-16T16:17:58Z

openshift_metrics/merge.py

+        report_start_date_dt = datetime.strptime(report_start_date, "%Y-%m-%d")
+        report_end_date_dt = datetime.strptime(report_end_date, "%Y-%m-%d")
        ignore_hours = outage_data.get_outages_during(
-            report_start_date, report_end_date, cluster_name
+            report_start_date_dt, report_end_date_dt, cluster_name


The current upstream code runs fine with report_start_date and report_end_date. Is there a reason you're converting them to datetime objects here?

QuanMPhm · 2025-12-16T16:22:56Z

openshift_metrics/merge.py

+        if RATE_CPU_SU is None:
+            raise ValueError("RATE_CPU_SU environment variable must be set")
+        if RATE_GPU_V100_SU is None:
+            raise ValueError("RATE_GPU_V100_SU environment variable must be set")
+        if RATE_GPU_A100SXM4_SU is None:
+            raise ValueError("RATE_GPU_A100SXM4_SU environment variable must be set")
+        if RATE_GPU_A100_SU is None:
+            raise ValueError("RATE_GPU_A100_SU environment variable must be set")
+        if RATE_GPU_H100_SU is None:
+            raise ValueError("RATE_GPU_H100_SU environment variable must be set")
+


In the upstream version of the code, an error is already raised by the Rates dataclass if the SU rates are None. I suppose these a bit more informative, but I would argue they are not entirely necessary and leads to more maintenance burden down the line.

QuanMPhm · 2025-12-16T16:31:50Z

openshift_metrics/merge.py

+    report_start_date_dt = datetime.strptime(report_start_date, "%Y-%m-%d").replace(
+        tzinfo=UTC
+    )
+    report_end_date_dt = datetime.strptime(report_end_date, "%Y-%m-%d").replace(


If you want to indicate that report_start_date and report_end_date are datetime objects, I would suggest using type annotation instead, which I think would be simpler to interpret than having to rename references and assignements.

Suggested change

report_start_date_dt = datetime.strptime(report_start_date, "%Y-%m-%d").replace(

tzinfo=UTC

)

report_end_date_dt = datetime.strptime(report_end_date, "%Y-%m-%d").replace(

report_start_date : datetime = datetime.strptime(report_start_date, "%Y-%m-%d").replace(

tzinfo=UTC

)

report_end_date : datetime = datetime.strptime(report_end_date, "%Y-%m-%d").replace(

tzinfo=UTC

)

Is there another reason why you renamed these variables?

QuanMPhm · 2025-12-16T16:32:43Z

openshift_metrics/merge.py

+    if report_start_date_dt.month != report_end_date_dt.month:
+        logger.warning("The report spans multiple months")
+        report_month += " to " + datetime.strftime(report_end_date_dt, "%Y-%m")


This if clause is no longer needed. It will cause an error with the call to nerc-rates in get_su_definitions

jimmysway requested review from QuanMPhm and naved001 October 15, 2025 14:19

jimmysway force-pushed the fix/144-consolidating-config branch from 39e759b to b1b589e Compare October 15, 2025 14:21

QuanMPhm requested changes Oct 16, 2025

View reviewed changes

Addresses CCI-MOC#144, Part 2: Adding Billing Rates to the config.py …

2967310

…and rewriting code references. Removed CLI arguments for setting rates

jimmysway force-pushed the fix/144-consolidating-config branch from b1b589e to 2967310 Compare November 11, 2025 15:25

Merge remote-tracking branch 'upstream/main' into fix/144-consolidati…

d7d27cc

…ng-config

jimmysway and others added 2 commits December 9, 2025 15:13

Resolved merge conflicts

938dfd6

Merge branch 'main' into fix/144-consolidating-config

7fe5f96

jimmysway requested a review from QuanMPhm December 15, 2025 19:26

QuanMPhm requested changes Dec 16, 2025

View reviewed changes

		SU_NAMES = ["GPUV100", "GPUA100", "GPUA100SXM4", "GPUH100", "CPU"]
		RESOURCE_NAMES = ["vCPUs", "RAM", "GPUs"]

		USE_NERC_RATES_ENV = os.getenv("USE_NERC_RATES")
		USE_NERC_RATES = USE_NERC_RATES_ENV.lower() == "true" if USE_NERC_RATES_ENV else None

	USE_NERC_RATES_ENV = os.getenv("USE_NERC_RATES")
	USE_NERC_RATES = USE_NERC_RATES_ENV.lower() == "true" if USE_NERC_RATES_ENV else None
	USE_NERC_RATES = os.getenv("USE_NERC_RATES", "false").lower() == "true"

Adding Billing Rates to the config.py and rewriting references #159

Are you sure you want to change the base?

Adding Billing Rates to the config.py and rewriting references #159

Conversation

jimmysway commented Oct 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naved001 commented Nov 10, 2025

Uh oh!

jimmysway commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QuanMPhm commented Nov 18, 2025

Uh oh!

QuanMPhm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jimmysway commented Nov 11, 2025 •

edited

Loading

QuanMPhm left a comment •

edited

Loading