Use nerc-rates for billing #89
Conversation
This add the necessary code to get rates from the nerc-rates repo or specify them from the command line. The produce report script is updated to use nerc-rates. Addtionaly the tests were updated since the function `write_metrics_by_namespace` now takes an additional argument. All the calls to that function have been updated to keep things more readable.
naved001
force-pushed
the
use-nerc-rates
branch
from
73324ce to
c343ed8
Compare
openshift_metrics/merge.py
Outdated
| report_month = datetime.strftime(report_start_date, "%Y-%m") | ||
|
|
||
| if args.use_nerc_rates: | ||
| nerc_rates = load_from_url() |
There was a problem hiding this comment.
This tripped me up for a bit because I was wondering how load_from_url() doesn't take any params until I realized it comes from nerc_rates. Maybe importing nerc rates and calling nerc_rates.load_from_url() would be better like you are doing for get_value_at() below.
There was a problem hiding this comment.
@hakasapl Good point! I was also overwriting the name of the module which would have been confusing even if it didn't break things. I have fixed this now.
| parser.add_argument("--rate-cpu-su", type=Decimal) | ||
| parser.add_argument("--rate-gpu-v100-su", type=Decimal) | ||
| parser.add_argument("--rate-gpu-a100sxm4-su", type=Decimal) | ||
| parser.add_argument("--rate-gpu-a100-su", type=Decimal) |
There was a problem hiding this comment.
The way things are currently structured, it's legal to run with, say, --use-nerc-rates --rate-cpu-su=1, even though the second argument would be ignored. This would be a good use case for a mutually exclusive group. Alternately, should it be possible to override individual rates from nerc-rates with a command line options?
Since none of the rate options have defaults, they will be 0 if unspecified. Practically, that means that when not using --use-nerc-rates, all the rate options need to be set explicitly. Should we raise an error if only some of them are set explicitly?
There was a problem hiding this comment.
@larsks right, I did look into using mutually exclusive group but couldn't get it to work with my use case. I'll take another look at it.
Since none of the rate options have defaults, they will be 0 if unspecified.
I tested this behavior and it's actually NoneType if unspecified and then when I cast it to Decimal it'll throw an error.
File "/Users/naved/work/openshift-usage-scripts/openshift_metrics/merge.py", line 116, in main
gpu_a100=Decimal(args.rate_gpu_a100_su),
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: conversion from NoneType to Decimal is not supported
So it raises an ugly error if not all of the rates are set but it doesn't silently set it 0. And now that I think about it, I actually don't need to manually cast it as Decimal since the type is already specified. So, if one of the rates is not specified the program will fail at a later stage when it tries to use this NoneType rate (which would be ugly).
I did think of these scenarios when I was creating this PR and I had a function to manually validate all these conditions but I felt it's getting too complicated for a tool that will mostly be run automatically. But I'd be happy to add the validation back. I am going to first see if I can use the mutually exclusive groups, if not I'll had some custom argument validation for these.
There was a problem hiding this comment.
I felt it's getting too complicated for a tool that will mostly be run automatically
Yeah, there's a reason I marked the PR as "approved" :).
I did look into using mutually exclusive group but couldn't get it to work with my use case
On second thought, it might be tricky to make this behave the way I was thinking.
There was a problem hiding this comment.
Yeah, there's a reason I marked the PR as "approved" :).
In that case, I am going to go ahead and merge this.
This add the necessary code to get rates from the nerc-rates repo or specify them from the command line. The "produce report" script is updated to use nerc-rates.
Addtionaly the tests were updated since the function
write_metrics_by_namespacenow takes an additional argument. All the calls to that function have been updated to keep things more readable.