Python raven #1685
Python raven #1685
Conversation
PaulTalbot-INL
requested review from
joshua-cogliati-inl,
mandd,
wangcj05,
dylanjm and
Jimmy-INL
| Perform Unit Tests for running RAVEN in Python workflows. | ||
| SQA Note: unittest requires all test methods start with test_, so we excercise SQA exception | ||
| SQA Note: ALL tests take no input and provide no output, so these are omitted in the | ||
| test_* method descriptions. |
There was a problem hiding this comment.
Note these SQA notes ...
|
Apparently the ForwardSampling inputs from the Workshop are constructed, not in the repo. I think I'll make a faster-running "basic" RAVEN framework test workflow to use instead of the workshop workflow. |
| import utils.TreeStructure as TS | ||
| from BaseClasses import MessageUser | ||
|
|
||
| class Raven: |
There was a problem hiding this comment.
Is it possible to combine Driver and this class? I mean to create a class that be used by both.
There was a problem hiding this comment.
There's definitely some commonality that could be further abstracted; I think some utilities for drivers might be in order.
I initially was going to move Driver.py into a Drivers folder in framework, but that caused a lot of things to complain loudly, so for now it makes sense to have Driver where it's at until we are more sure of how we want the custom usage to go.
One of my next action items would be to abstract some of the library loading and checking so that it can be commonly used between the default driver and custom drivers such as this python-based one.
There was a problem hiding this comment.
I added some utility abstraction in the latest push
|
I will add my review tomorrow night if it is okay for you guys. From a quick check (not knowing the future plan for this "driver" system), I do not see what's the scope of this additional wrapper, since mostly it is a mirror of what the What's the final goal of this development ? (I ask just to have some context for tailoring the review to what this will need to be and not what it is right now). |
The additions in this PR don't require any library changes. It's already on the table to do a lib update, and that would be a better time to consider doing Jupyter compatibility. |
It's quite a bit different from the Driver.py module, in that it offers an interface for handling RAVEN from within a Python environment such as a Jupyter notebook or as part of other Python-based workflows. In the design meeting we had, we identified one of the most flexible and useful extension of RAVEN towards making it a part of cloud-based computing applications was to allow it to be accessed and integrated via a Python controller, rather than exclusively from the command line. This might have been possible for a dedicated individual before, but this establishes a straightforward API for doing so, and is the first step in making this feature fully-fledged. This would also make possible something @aalfonsi and @crisrab talked about many times before: holding objects in memory after a run is complete so that the user can explore them, and maybe in the future add steps to do additional sampling, postprocessing, etc. |
There was a problem hiding this comment.
@PaulTalbot-INL: I noticed several changes have been added since my last review. I have few comments here:
- I am not sure what is the real advantage by moving the content of Driver.py into DriverUtils.py (e.g., adding further abstractions). It appears to me i just adds more code
- the core of your PR are the methods getEntity and loadWorkflowFromFile which call directly methods of Simulation class. These methods can be actually part of the simulation class.
I played a bit with you PR and I tried to make some changes on my local branch followed my observations in 2) in a workflow as follows:
from Simulation import Simulation
simulation = Simulation(frameworkDir)
simulation.loadWorkflowFromFile(targetWorkflow)
returnCode = simulation.run()
results = simulation.getEntity('DataObjects', 'results')
The methods getEntity and loadWorkflowFromFile were actually copied and pasted into theSimulation class. I think it is a simpler structure.
|
in reply to @mandd ... (1) was added on request from @wangcj05 above. (2) the intent is to provide a protected, clean API for the user; Simulation is fairly involved and absolutely expects the workflow to be run once and from the command line. Building in an API layer in between will allow us to accelerate the use of RAVEN by simplifying the interactions and protecting the parts of RAVEN that really shouldn't be tinkered with for successful operation. This will also make documentation very easy; imagine documenting Simulation as an externally-importable object in generic Python right now! |
|
Apologies for the late reply; given my (poor) experience with Simulation.py, I had to do some homework and some testing. Would it suffice to create a class that wraps what we want a user to see? |
|
@mandd it seems we are on the same page...I was actually thinking the same thing and did something very similar locally (without changing the simulation)...Tonight (just for fun) I will try to post the code for you guys to discuss |
Isn't that what we did here? I don't see a mechanical difference between what's in this PR and what you put down; maybe I missed something. I think we all have the same idea in mind. |
@aalfonsi Do you have any update on this? Are you still going to push your version of raven as library to the repository? |
|
@wangcj05 I added my modifications in here https://github.com/idaholab/raven/tree/test_simulation It is nothing special (mostly following Paul's idea, but without a new wrapper)....It would require cleaning, a better way to import the Driver (just to set up the right environment) and to scan the Simulation.py to protect the methods that can not be called from outside (basically making the Simulation the API for external integration). |
| @@ -1,78 +0,0 @@ | |||
| # Copyright 2017 Battelle Energy Alliance, LLC | |||
There was a problem hiding this comment.
This test is redundant with the PythonRaven unit test now.
|
Job Test Fedora 32 on 5b5a400 : invalidated by @PaulTalbot-INL Errored by Ray: "ValueError: Ray component metrics_export is trying to use a port number 52953 that is used by other components. Port information: {''gcs'': [], ''object_manager'': [], ''node_manager'': [], ''gcs_server'': [], ''client_server'': [], ''dashboard'': [], ''dashboard_agent'': [52953], ''metrics_export'': [52953], ''redis_shards'': [], ''worker_ports'': []} If you allocate ports, please make sure the same port is not used by multiple components." |
|
Job Test qsubs sawtooth on 5b5a400 : invalidated by @PaulTalbot-INL First 4 tests unexpectedly timed out, no others did. |
PaulTalbot-INL
dismissed
mandd’s stale review
These review comments have been addressed in the RAVEN design meeting. Notes are being provided on individual comment resolutions.
There was a problem hiding this comment.
I have a couple of minor comments for your consideration. @PaulTalbot-INL
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| """ | ||
| Demonstrate running RAVEN in Python workflows. |
There was a problem hiding this comment.
@PaulTalbot-INL I think we should keep this test for now. It tests the step by step execution capability. When you have a new design, you are very welcome to change or remove it.
There was a problem hiding this comment.
It does not work now; I don't think it's worth maintaining since it's been replaced by the TestPythonRaven and DemoPythonRaven tests. I don't think it's worth our time fixing this test, since Andrea offered it as an alternative to having the driver wrapper (PythonRaven) and use Simulation directly, which we decided we didn't want to encourage as part of the design meeting.
There was a problem hiding this comment.
Ok, it sounds good to me. By the way, I do not think it will take a lot of time to fix it.
| ##TODO REMOVE PP3 WHEN RAY IS AVAILABLE FOR WINDOWS | ||
| utils.add_path_recursively(os.path.join(frameworkDir,'contrib','pp')) |
There was a problem hiding this comment.
This is just a question, are we still use 'pp' for windows or do we switched to ray? @joshua-cogliati-inl
|
Job Test qsubs sawtooth on c055921 : invalidated by @PaulTalbot-INL unexpected test failure only in sawtooth checking consistency |
|
FYI @wangcj05 (as plugin developer notification) it appears this breaks something in the SR2ML and Logos tests, possibly the implicit importing of "Driver" to load the RAVEN environment. Examples of fixes for that can be seen in the unit test fixes in this PR. |
Thanks @PaulTalbot-INL, I will start to update the plugins after the merge of this PR. |
|
checklist is good. |
|
@PaulTalbot-INL I have merged your PR, thanks for your contributions! |
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
Starts on #1682
What are the significant changes in functionality due to this change request?
Adds a Python "driver" instance that wraps RAVEN simulations.
NOTE does not currently support multiple subsequent runs on the same workflow without re-initializing.
For Change Control Board: Change Request Review
The following review must be completed by an authorized member of the Change Control Board.
<internalParallel>to True.raven/tests/framework/user_guideandraven/docs/workshop) have been changed, the associated documentation must be reviewed and assured the text matches the example.