add option CHARGE_ACCOUNT to charge compute time if different from PROJECT #1718

sarich · 2017-08-15T17:06:53Z

This option can be set by environment variable, in config_machines.xml, or
with xmlchange.
Helpful because PROJECT can be used for directory names that don't match
the name of the charge account.

Addresses issue #1588

… PROJECT This option can be set by environment variable or in config_machines.xml Helpful because PROJECT can be used for directory names that don't match the name of the charge account.

jgfouca

See inline

jgfouca · 2017-08-15T18:21:07Z

cime/scripts/create_newcase

@@ -59,6 +59,7 @@ OR
    parser.add_argument("--project", "-project",
                        help="Specify a project id")

+


Plz remove unnecessary newline.

jgfouca · 2017-08-15T18:21:15Z

cime/scripts/create_test

@@ -183,6 +183,7 @@ OR
                        "Used for accounting when on a batch system."
                        "The default is user-specified environment variable PROJECT")

+


jgfouca · 2017-08-15T18:25:17Z

cime/scripts/lib/CIME/utils.py

+    1. Environment variable CHARGE_ACCOUNT
+    2. File $HOME/.cime/config
+    3. config_machines.xml (if machobj provided)
+    4. default to same value as PROJECT


This is intricate enough to warrant a unit test. A couple simple doctests would suffice.

sarich · 2017-08-15T18:32:25Z

whitespace fixed. I'd be happy to make a unit test for it, but haven't done these before. Is there a similar existing test I can base it off of?

…

On Tue, Aug 15, 2017 at 1:25 PM, James Foucar ***@***.***> wrote: ***@***.**** requested changes on this pull request. See inline ------------------------------ In cime/scripts/create_newcase <#1718 (comment)>: > @@ -59,6 +59,7 @@ OR parser.add_argument("--project", "-project", help="Specify a project id") + Plz remove unnecessary newline. ------------------------------ In cime/scripts/create_test <#1718 (comment)>: > @@ -183,6 +183,7 @@ OR "Used for accounting when on a batch system." "The default is user-specified environment variable PROJECT") + Ditto ------------------------------ In cime/scripts/lib/CIME/utils.py <#1718 (comment)>: > @@ -618,6 +618,36 @@ def get_project(machobj=None): logger.info("No project info available") return None +def get_charge_account(machobj=None): + """ + Hierarchy for choosing CHARGE_ACCOUNT: + 1. Environment variable CHARGE_ACCOUNT + 2. File $HOME/.cime/config + 3. config_machines.xml (if machobj provided) + 4. default to same value as PROJECT This is intricate enough to warrant a unit test. A couple simple doctests would suffice. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1718 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEs_usoS2w0X5BB4f505o5ncFyIMCNnAks5sYeKZgaJpZM4O32U6> .

jgfouca · 2017-08-15T18:54:35Z

@sarich

Since the function uses a Machine object, the unit test would probably look most like some of the tests in machines.py. Look for <<<

sarich · 2017-08-16T15:18:03Z

I've added a couple of tests, let me know if you have any more suggestions

jgfouca · 2017-08-16T16:11:05Z

cime/scripts/lib/CIME/utils.py

+    4. default to same value as PROJECT
+
+    >>> import CIME
+    >>> import CIME.XML.machines


This looks perfect, thanks.

jgfouca · 2017-08-16T16:12:52Z

cime/scripts/lib/CIME/utils.py

+    >>> os.environ["CHARGE_ACCOUNT"] = "ChargeAccount"
+    >>> get_charge_account(machobj)
+    'ChargeAccount'
+    >>> reset_cime_config()


Oh, one quick note: I think you should unset CHARGE_ACCOUNT here instead of resetting cime_config. We don't want this env variable to pollute other tests.

jgfouca · 2017-08-18T17:45:36Z

Approved. Waiting for @mrnorman

mrnorman · 2017-08-22T12:20:08Z

Sorry, I've been out sick for a couple of weeks. Also Titan's down today. So hopefully I can test this tomorrow.

mrnorman · 2017-08-24T16:47:54Z

When does CHARGE_ACCOUNT need to be defined in the environment? At ./create_newcase, ./case.setup, or ./case.submit? It would be convenient if it only has to be specified at ./case.submit if that's possible. I'm currently testing it with it defined at ./create_newcase level to make sure it works for me on Titan.

sarich · 2017-08-24T17:10:26Z

the CHARGE_ACCOUNT environment variable is only checked when running ./create_newcase (or ./create_test), when it builds the env_batch.xml file

…

On Thu, Aug 24, 2017 at 11:47 AM, Matt Norman ***@***.***> wrote: When does CHARGE_ACCOUNT need to be defined in the environment? At ./create_newcase, ./case.setup, or ./case.submit? It would be convenient if it only has to be specified at ./case.submit if that's possible. I'm currently testing it with it defined at ./create_newcase level to make sure it works for me on Titan. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1718 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEs_ugs_XTM6Pm-7nldNerMHkP5PubFDks5sbak8gaJpZM4O32U6> .

mrnorman · 2017-08-24T17:13:04Z

Well, it ran successfully for me on Titan.

mrnorman

Works on Titan for csc190acme.

jgfouca · 2017-08-24T17:13:59Z

OK, merging

sarich · 2017-08-24T17:14:17Z

You can also use xmlchange to set this value

…

On Thu, Aug 24, 2017 at 12:10 PM, Jason Sarich ***@***.***> wrote: the CHARGE_ACCOUNT environment variable is only checked when running ./create_newcase (or ./create_test), when it builds the env_batch.xml file On Thu, Aug 24, 2017 at 11:47 AM, Matt Norman ***@***.***> wrote: > When does CHARGE_ACCOUNT need to be defined in the environment? At > ./create_newcase, ./case.setup, or ./case.submit? It would be convenient if > it only has to be specified at ./case.submit if that's possible. I'm > currently testing it with it defined at ./create_newcase level to make sure > it works for me on Titan. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#1718 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AEs_ugs_XTM6Pm-7nldNerMHkP5PubFDks5sbak8gaJpZM4O32U6> > . >

) add option CHARGE_ACCOUNT to charge compute time if different from PROJECT This option can be set by environment variable, in config_machines.xml, or with xmlchange. Helpful because PROJECT can be used for directory names that don't match the name of the charge account. Addresses issue #1588 * origin/sarich/scripts/feature-charge-account: unset CHARGE_ACCOUNT environment variable in tests Add simple doctests to get_charge_account whitespace edits add option CHARGE_ACCOUNT to charge compute time to if different from PROJECT

jgfouca · 2017-08-24T17:16:42Z

Merged to next, I'll merge to master if it doesn't break anything.

worleyph · 2017-08-24T17:17:28Z

@mrnorman , what experiment did you run? Do you know where the performance data was put? In $PROJECT or $CHARGE_ACCOUNT project directory? ($PROJECT would be my preference.)

sarich · 2017-08-24T17:22:08Z

It should go to $PROJECT directory, $CHARGE_ACCOUNT is only used to set the account flag in the queue submission.

…

On Thu, Aug 24, 2017 at 12:17 PM, worleyph ***@***.***> wrote: @mrnorman <https://github.com/mrnorman> , what experiment did you run? Do you know where the performance data was put? In $PROJECT or $CHARGE_ACCOUNT project directory? ($PROJECT would be my preference.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1718 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEs_ukAXvc8AIaBcBBGD9fEwbthr7T1xks5sbbAqgaJpZM4O32U6> .

mrnorman · 2017-08-24T17:24:07Z

@worleyph I ran FC5AV1C-L with csc190acme, specifying CHARGE_ACCOUNT=csc190acme in the environment and -project csc190 in the create_newcase. It successfully ran.

mrnorman · 2017-08-24T17:25:24Z

/lustre/atlas/proj-shared/csc190/performance_archive/imn/test_charge_project is where the performance data went.

worleyph · 2017-08-24T17:28:25Z

Thanks. Looks good.

mrnorman · 2017-08-28T14:48:14Z

When will this be merged to master?

…1718) add option CHARGE_ACCOUNT to charge compute time if different from PROJECT This option can be set by environment variable, in config_machines.xml, or with xmlchange. Helpful because PROJECT can be used for directory names that don't match the name of the charge account. Addresses issue #1588 * origin/sarich/scripts/feature-charge-account: unset CHARGE_ACCOUNT environment variable in tests Add simple doctests to get_charge_account whitespace edits add option CHARGE_ACCOUNT to charge compute time to if different from PROJECT

jgfouca · 2017-08-28T16:50:04Z

@mrnorman done.

mrnorman · 2017-08-29T14:46:15Z

Sweet!

…1718) add option CHARGE_ACCOUNT to charge compute time if different from PROJECT This option can be set by environment variable, in config_machines.xml, or with xmlchange. Helpful because PROJECT can be used for directory names that don't match the name of the charge account. Addresses issue #1588 * origin/sarich/scripts/feature-charge-account: unset CHARGE_ACCOUNT environment variable in tests Add simple doctests to get_charge_account whitespace edits add option CHARGE_ACCOUNT to charge compute time to if different from PROJECT

add option CHARGE_ACCOUNT to charge compute time to if different from…

3ec85f6

… PROJECT This option can be set by environment variable or in config_machines.xml Helpful because PROJECT can be used for directory names that don't match the name of the charge account.

sarich added BFB PR leaves answers BFB CIME Machine Files labels Aug 15, 2017

rljacob assigned jgfouca Aug 15, 2017

rljacob requested a review from mrnorman August 15, 2017 17:38

jgfouca requested changes Aug 15, 2017

View reviewed changes

whitespace edits

af58e62

Add simple doctests to get_charge_account

a75aa60

jgfouca reviewed Aug 16, 2017

View reviewed changes

unset CHARGE_ACCOUNT environment variable in tests

a8d4242

sarich mentioned this pull request Aug 18, 2017

Handle projects and subprojects in machine/batch variables. ESMCI/cime#1332

Closed

jgfouca approved these changes Aug 18, 2017

View reviewed changes

mrnorman approved these changes Aug 24, 2017

View reviewed changes

jgfouca merged commit a8d4242 into master Aug 28, 2017

jgfouca deleted the sarich/scripts/feature-charge-account branch August 28, 2017 16:49

jhkennedy mentioned this pull request Oct 29, 2018

CHARGE_ACCOUNT no longer allows for sub-projects ESMCI/cime#2886

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add option CHARGE_ACCOUNT to charge compute time if different from PROJECT #1718

add option CHARGE_ACCOUNT to charge compute time if different from PROJECT #1718

sarich commented Aug 15, 2017

jgfouca left a comment

jgfouca Aug 15, 2017

jgfouca Aug 15, 2017

jgfouca Aug 15, 2017

sarich commented Aug 15, 2017 via email

jgfouca commented Aug 15, 2017

sarich commented Aug 16, 2017

jgfouca Aug 16, 2017

jgfouca Aug 16, 2017

jgfouca commented Aug 18, 2017

mrnorman commented Aug 22, 2017

mrnorman commented Aug 24, 2017

sarich commented Aug 24, 2017 via email

mrnorman commented Aug 24, 2017

mrnorman left a comment

jgfouca commented Aug 24, 2017

sarich commented Aug 24, 2017 via email

jgfouca commented Aug 24, 2017

worleyph commented Aug 24, 2017

sarich commented Aug 24, 2017 via email

mrnorman commented Aug 24, 2017

mrnorman commented Aug 24, 2017

worleyph commented Aug 24, 2017

mrnorman commented Aug 28, 2017

jgfouca commented Aug 28, 2017

mrnorman commented Aug 29, 2017

		@@ -59,6 +59,7 @@ OR
		parser.add_argument("--project", "-project",
		help="Specify a project id")

		@@ -183,6 +183,7 @@ OR
		"Used for accounting when on a batch system."
		"The default is user-specified environment variable PROJECT")

add option CHARGE_ACCOUNT to charge compute time if different from PROJECT #1718

add option CHARGE_ACCOUNT to charge compute time if different from PROJECT #1718

Conversation

sarich commented Aug 15, 2017

jgfouca left a comment

Choose a reason for hiding this comment

jgfouca Aug 15, 2017

Choose a reason for hiding this comment

jgfouca Aug 15, 2017

Choose a reason for hiding this comment

jgfouca Aug 15, 2017

Choose a reason for hiding this comment

sarich commented Aug 15, 2017 via email

jgfouca commented Aug 15, 2017

sarich commented Aug 16, 2017

jgfouca Aug 16, 2017

Choose a reason for hiding this comment

jgfouca Aug 16, 2017

Choose a reason for hiding this comment

jgfouca commented Aug 18, 2017

mrnorman commented Aug 22, 2017

mrnorman commented Aug 24, 2017

sarich commented Aug 24, 2017 via email

mrnorman commented Aug 24, 2017

mrnorman left a comment

Choose a reason for hiding this comment

jgfouca commented Aug 24, 2017

sarich commented Aug 24, 2017 via email

jgfouca commented Aug 24, 2017

worleyph commented Aug 24, 2017

sarich commented Aug 24, 2017 via email

mrnorman commented Aug 24, 2017

mrnorman commented Aug 24, 2017

worleyph commented Aug 24, 2017

mrnorman commented Aug 28, 2017

jgfouca commented Aug 28, 2017

mrnorman commented Aug 29, 2017