-
Notifications
You must be signed in to change notification settings - Fork 3
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
Added processing and exporting for Lenovo SU Types #14
Conversation
process_report/tests/unit_tests.py
Outdated
'Project - Allocation': ['ProjectA', 'ProjectB', 'ProjectC', 'ProjectD', 'ProjectE', 'ProjectF'], | ||
'Institution': ['A', 'B', 'C', 'D', 'E', 'F'], | ||
'SU Hours (GBhr or SUhr)': [1, 10, 100, 4, 432, 10], | ||
'SU Type': ['OpenShift GPUA100SXM4', 'CPU', 'OpenShift GPUA100SXM4', 'OpenStack GPUA100SXM4', 'CPU', 'GPU'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of GPU can we put GPUA100, and instead of a second CPU can we put GPUV100
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joachimweyl For the purpose of this test, I don't think replacing the string GPU
with GPUA100
would make a difference, since to know if a project used a Lenovo SU type, the code tries to match the SU Type exactly to either OpenShift GPUA100SXM4
or OpenStack GPUA100SXM4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not supposed to, but the point of a test is to test all possibilities right?
To be clear we should put OpenShift GPUA100
and OpenStack GPUV100
. and CPU
should actually be OpenShift CPU
or OpenStack CPU
. We also might want to test that it properly combines OpenStack GPUA100SXM4
so maybe replace one of the other ones with that instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joachimweyl What do you mean by properly combines OpenStack GPUA100SXM4
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"calculates" not combines, sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the list of SU Types being tested.
I don't know Python well enough to check your code but the logic is sound. |
98b4b68
to
ce3fbe0
Compare
@naved001 @joachimweyl I've addressed all of comments so far in both this PR and the Lenovo PR, except for ones involving @knikolla |
ce3fbe0
to
3e85942
Compare
3e85942
to
aec6733
Compare
Closes #13, I have added an export function
export_lenovo()
which exports an invoice CSV following the specifications in the related issue. The list of SU types considered to be Lenovo and the SU Charge of $1 have been put into constant values defined in the function.This PR is blocked by #12.