-
Notifications
You must be signed in to change notification settings - Fork 37
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
Switch from run scripts to compass run
command
#76
Conversation
cba65ad
to
9192379
Compare
@mark-petersen and @vanroekel, this change was suggested by @matthewhoffman in his review but we decided to wait until after #28 to discuss it. However, it would be really good to have it in soon if we want to make this switch because:
What I'm looking for in a review is for each of you to run:
Let me know if:
|
@xylar, sure! can do. Will try to do this by early next week. |
@xylar just tried this on anvil. the test case worked great this way, but when I tried running the nightly I get an error
Is it expected to try visualize even from a node of anvil? For completeness my setup command is
|
Thanks @vanroekel. That's odd because I always run on Anvil compute nodes and I've never seen this before. I'll make sure I can reproduce it. |
@vanroekel, I haven't been able to reproduce this. Here is what I did and maybe you can tell me if anything stands out as obviously different. I'm running with intel18 libraries, following E3SM:
I built the code using the submodule in the
I used
I didn't make a config file because that isn't necessary for supported machines. Instead, I did:
In a new terminal (just to make sure modules and conda environments are reset), I did:
On the compute node, I did:
I notice from the log file in your run |
This fits the style of the package much better and does not require writing python scripts from templates. If you are in a test case or step, you now simply run: ``` python -m compass run ``` If you want to run a suite, from the root of the work directory, run: ``` python -m compass run <suite> ```
Add some clarity about inputs and outputs to steps, and on where super().run() needs to get called in a test case's overridden version of this method. Add docstrings for run_step() and run_test_case()
This makes more conceptual sense than having them in the suite, testcase and step modules because they all get called only when `compass run` gets called.
9192379
to
5d79521
Compare
Thanks for the reply @xylar and your detailed instructions. I was pretty convinced this was on my end. I will try again following your instructions in the comment above later today, but to answer your question, I request nodes as you write in your instructions, i.e.
|
Thanks @vanroekel. Could you share the contents of |
I took another shot at this and did get past the weird errors, which I think were due to a very out of date
This looks like an issue in grabbing the compass version? All the QU240 logs have this error. I've used the same modules and conda load command as you list above. The nightly directory is
However, this appears unrelated to this PR, I just tried with compass/master and get the same error. This has to be on my end still. I'm happy to approve this PR based on the successful tests, if that is okay with you. I think the |
@vanroekel, thanks very much for testing again! The issue you are seeing above was fixed in #79. I verified that I rebased this branch onto |
@xylar I cleaned my repo and reset to master and then reset to this PR, in both cases the nightly completed successfully. I must have missed a reset --hard in previous testing. I will approve as it worked perfectly. Sorry for the false alarms, and thanks for helping me work through them! |
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.
approved based on visual inspection and successful testing on anvil of baroclinic channel and nightly test suite. Thanks @xylar!
Don't try to delete the script file (which we no longer create) during suite clean-up.
6cd5086
to
2a23f9b
Compare
@vanroekel, thanks for reviewing this so promptly, for putting up with the moving target, and for uncovering some odd errors we'll need to keep an eye out for in case they occur again. |
@xylar , I have one other comment about these changes. I saw you first added the run method to each class and then later moved the run function for each operation (step, testcase, suite) into the run module. I can see arguments for either organization. I think I agree with your choice to move them to the run module. However, the one exception would be if there was ever a reason to have a child class override the default run method. I cannot think of a situation where one might want to do that (and in fact it sounds dangerous/undesirable to allow it). Is that your assessment as well? |
These are functions, not methods, so they aren't part of the class and are not available to be overridden. I do not think we would ever want that. To me, that is a strong reason to take them out of the respective modules where the base classes are defined because there is a chance folks would expect them to be part of the classes, whereas they are part of the infrastructure not available for altering. |
Thanks, @xylar . Yes, I see they are functions now - I was just thinking through the alternate possibility of methods in the respective classes. But we are both in agreement that there is use (or desire) for that design choice, so the way you've implemented it here makes sense. I don't have any other comments on the PR. |
@matthewhoffman, great, thanks for the discussion. I completely agree that it's worth having. In general, it's really helpful to have you taking a careful look at these design choices and giving feedback. It really makes if feel like I'm not developing in my own bubble and helps to keep the design consistent. |
@xylar, in the current set-up, it is obvious to the user that the run steps are contained in run.py, and it is easy to copy from that script to the command line, or to only run parts of it. In this new setup, how do we see and alter the steps? Is there a way to do that locally in the run directory? |
No there is not, but that isn't something that changed with this PR, that was already true of #28. There was a I agree with you that this is likely to be a feature we want. I don't entirely agree that the old scripts were easy to copy from and edit but that was possible if you wanted to dig into the script. Would you be willing to make an issue with this feature request and we can address it in another PR? I could imagine a few different ways to handle this and we should discuss it before I try implementing anything. |
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 on grizzly. Ran the nightly test suite with ocean/develop
. I agree that this format for run fit better into the python command format. Thanks!
Thanks very much, @mark-petersen. Feel free to make that issue I mentioned above. |
To run test cases and steps, rather than
./run.py
, you now run:If you want to run a suite, from the root of the work directory, run:
instead of
./<suite>.py
.The
compass run
command fits the style of the package much better than./run.py
and does not require writing python scripts from templates.This merge also updates some docstrings for better clarity on what "inputs" and "outputs" of a step refer to, and on where
super().run()
needs to get called in a test case's overridden version of therun()
method.The merge also adds docstrings for run_step() and run_test_case(). The three
run_*()
functions have been moved to therun
module, which makes more conceptual sense than having them in thesuite
,testcase
andstep
modules because they all get called only whencompass run
gets called.closes #71