Skip to content
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

Do not get cwd in arg defaults #402

Merged
merged 2 commits into from
Aug 16, 2016
Merged

Do not get cwd in arg defaults #402

merged 2 commits into from
Aug 16, 2016

Conversation

billsacks
Copy link
Member

In two functions - check_lockedfiles and TestStatus.init - there was
a directory argument whose default was given by os.getcwd(). However, my
understanding of python's default arguments is that these defaults were
evaluated at the time the function was loaded, not at the time the
function was called.

This PR changes the behavior so that os.getcwd is evaluated at the time
the functions are called, if the respective arguments are not provided.

Without this change, I was getting failures in a few tests in
scripts_regression_tests on yellowstone, with messages like this:

======================================================================
FAIL: test_create_test_rebless_namelist (__main__.C_TestCreateTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./scripts_regression_tests.py", line 468, in test_create_test_rebless_namelist
    self.simple_test(True, "-g -n -b %s -t %s-%s" % (self._baseline_name, self._baseline_name, CIME.utils.get_utc_timestamp()))
  File "./scripts_regression_tests.py", line 459, in simple_test
    self.assertEqual(stat, 0, msg="COMMAND '%s' SHOULD HAVE WORKED\ncreate_test output:\n%s\n\nerrput:\n%s\n\ncode: %d" % (cmd, output, errput, stat))
AssertionError: COMMAND '/glade/p/work/sacks/cime/scripts/create_test cime_test_only_pass -g -n -b fake_testing_only_20160815_202538 -t fake_testing_only_20160815_202538-20160815_202538' SHOULD HAVE WORKED
create_test output:


errput:
shell-init: error retrieving current directory: getcwd: cannot access parent directories: No such file or directory
Traceback (most recent call last):
  File "/glade/p/work/sacks/cime/scripts/create_test", line 14, in <module>
    from CIME.test_scheduler import TestScheduler, RUN_PHASE
  File "/glade/p/work/sacks/cime/scripts/Tools/../../utils/python/CIME/test_scheduler.py", line 16, in <module>
    from CIME.test_status import *
  File "/glade/p/work/sacks/cime/scripts/Tools/../../utils/python/CIME/test_status.py", line 63, in <module>
    class TestStatus(object):
  File "/glade/p/work/sacks/cime/scripts/Tools/../../utils/python/CIME/test_status.py", line 65, in TestStatus
    def __init__(self, test_dir=os.getcwd(), test_name=None):
OSError: [Errno 2] No such file or directory

code: 1

The changes here seem to resolve that problem. It's not clear to me why
I sometimes ran into that problem and sometimes didn't, but this change
seems more "right" to me anyway, since it seems problematic to resolve
the current directory at the time the function happens to be loaded.

From a quick look, it looks like this change could, in principle, change
behavior for case_setup, which does:

os.chdir(caseroot)

before calling:

        check_lockedfiles()

I cannot find any instances of TestStatus.init being called without
an explicit test_dir argument, so it doesn't look like that behavior
would be affected by this change.

Test suite: scripts_regression_tests on yellowstone
Test baseline:
Test namelist changes:
Test status: bit for bit

All passed except for the same 4 failures that I got in testing master:

test_check_code (__main__.CheckCode) ... FAIL
test_b_full (__main__.E_TestTestScheduler) ... FAIL
test_save_timings (__main__.TestSaveTimings) ... FAIL
test_full_system (__main__.Z_FullSystemTest) ... FAIL

Fixes none

User interface changes?: none

Code review: none yet

For some reason, some tests in scripts_regression_tests were failing for me with
the old version of this code - maybe because default arguments are evaluated
when the function is defined (not when it is called)?

This new version seems more likely to be what's wanted - i.e., getting the
current working directory at the time when the function is called, not the time
when it's defined.

From a quick look, it looks like this default could be removed entirely and made
a required argument.

Test suite: None
Test baseline: N/A
Test namelist changes: N/A
Test status: N/A

Fixes: None

User interface changes?: No

Code review: None
For some reason, some tests in scripts_regression_tests were failing for me with
the old version of this code - maybe because default arguments are evaluated
when the function is defined (not when it is called)?

This new version seems more likely to be what's wanted - i.e., getting the
current working directory at the time when the function is called, not the time
when it's defined.

In particular, this seems like it could (in principle) be important in
case_setup, which does:

    os.chdir(caseroot)

before calling:

            check_lockedfiles()

Test suite: None
Test baseline: N/A
Test namelist changes: N/A
Test status: N/A

Fixes: None

User interface changes?: No

Code review: None
@jedwards4b jedwards4b merged commit f95584b into ESMCI:master Aug 16, 2016
@billsacks billsacks deleted the do_not_get_cwd_in_arg_default branch August 16, 2016 02:51
@jgfouca
Copy link
Contributor

jgfouca commented Aug 16, 2016

Thanks, I may have done this in some other places as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants