-
Notifications
You must be signed in to change notification settings - Fork 12
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
Make jenkins green... again #35
Conversation
… tests that have larger errors on c48. the goal is to get jenkins green again
…but this is a bit concerning
…, incorrectly implemented. instead use @Property + @lru_cache to achieve instance level caching
…. hopefully a fix in those will allow us to remove this override
… but be able to turn them on for cron plans
launch jenkins |
…nto a separate script
…tually using the baroclinic init
cp -rp .gt_cache* ${CACHE_DIR} | ||
# copying the cache is in a separate action (generache_cache.sh), | ||
# otherwise delete it | ||
if [ "${SAVE_CACHE}" != "true" ] ; then |
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.
should we call the action here if it is true?
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 was trying to leave open the flexibility to call other code, such as the regression tests, and then at the end the jenkins plan would call the generate_caches script. I think run_standalone should be sufficient for the cache, but have you seen evidence more things are compiled when we run the regression tests?
@@ -39,7 +39,7 @@ def parse_args() -> Namespace: | |||
"data_dir", | |||
type=str, | |||
action="store", | |||
help="directory containing data to run with", | |||
help="namelist configuration for model run", |
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.
this should probably also change its name?
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.
good catch, danger of manually undoing a change!
def get_experiment_info(data_directory: str) -> Tuple[str, bool]: | ||
config_yml = yaml.safe_load( | ||
open( | ||
data_directory + "/input.yml", |
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.
is both the nml and the yml file always copied over to have both?
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.
they should both be with the data now. We can eventually refer to one, but the Namelist class got a lot more hard coded... it's a bit of a hassle to check it for the test_case which might not exist. It's easier to read this for the moment. 'experiment' can indeed be derived from the folder or the yml
) | ||
# create a state from serialized data | ||
savepoint_in = serializer.get_savepoint("FVDynamics-In")[0] | ||
driver_object = fv3core.testing.TranslateFVDynamics([grid]) |
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.
where does this grid come from?
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.
lol how did this work at all?
fv3core/fv3core/grid/generation.py
Outdated
|
||
return wrapped | ||
|
||
# TODO: when every environment in python3.8, replace |
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.
we could even have:
def cached_property(f):
@property
@functools.lru_cache()
def wrapper(self, *args, **kwargs):
return f(self, *args, **kwargs)
return wrapper
here and then the only change would be to get rid of it once we've moved up. Not super necessary
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 got really sick of trying out decorators and them not working right... I can do try this if you want, though wouldn't mind pushing forward for this PR
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.
lgtm
…r layout other than 1,1" This reverts commit af06290.
Fixing various issues that are disrupting our tests, including: