-
Notifications
You must be signed in to change notification settings - Fork 38
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
Override constrain_resources()
in individual steps
#481
Conversation
Testcase.run()
and instead override constrain_resources()
in individual stepsconstrain_resources()
in individual steps
TestingI ran the I have also run the remaining test groups that aren't part of the Tests from test group run and are BFB (if there is a baseline comparison):
Manually changing resources before calling
|
@vanroekel and @sbrus89, this should address the issue with the Tides test group discussed in #456 (comment) |
e41a1e9
to
716aad9
Compare
8d72783
to
960d0fc
Compare
@sbrus89 and @vanroekel, I'm not quite done testing Hurricane and Tides because they haven't run to completion but I did verify that I could change the resources for both init and forward at runtime. I would appreciate having you each test as well, though, just to make sure the issue if fixed. |
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 tested the hurricane and tides test case on chrysalis and confirmed a change to ntasks in the cfg file changed the number of cores used in the run. Thanks @xylar!
Seeing #482 but otherwise everything is working. |
Thanks @vanroekel. |
@sbrus89, I think @vanroekel's testing is sufficient. I'm going to merge. |
This merge drops calls to
Testcase.run()
and instead overridestep.constrain_resources()
in individual steps.We have deprecated
Testcase.run()
because its use is not compatible with the task parallelism approach we (@altheaden and I) are developing. Instead, the individual steps in a test case that need to set the number of tasks or cpus per task should overrideconstrain_resources()
, setself.ntasks
,self.min_tasks
, etc., and then callsuper().constrian_resources()
.In the near future (again related to task parallelism), it will also be useful to override
step.runtime_setup()
for tasks like creating graph partitions before running an MPAS component. In anticipation of this change and to be consistent with the deprecation warning aboutTestcase.run()
, this merge adds a call toruntime_setup()
when a step gets run. Currently, no steps override this function.Checklist
api.rst
) has any new or modified class, method and/or functions listedTesting
in this PR) any testing that was used to verify the changes