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

NEAMS Workbench #85

Open
wants to merge 22 commits into
base: development
Choose a base branch
from
Open

Conversation

zhieejhia93
Copy link
Contributor

@zhieejhia93 zhieejhia93 commented Jan 4, 2023

Description

This MR allows WATTS to be run with NEAMS Workbench

Fixes # (84)

Checklist:

  • My code follows the style guidelines
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have updated the CHANGELOG.md file (if applicable)
  • I have successfully run examples that may be affected by my changes

In the list of `Application Options`, input the path of `watts_ui.py` to `Executable`.
The file should exist by default in `/watts/src/watt_ui/`. Next, click `Load Grammar`.
Click `OK` to complete the setup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the setup worked great - very straightforward.

watts{
% Change the working directory to the directory where the input files are stored. This is necessary
% due to how WATTS copies input files to the temporary working directory.
workflow_dir = "/Users/zhieejhiaooi/Documents/ANL/watts-devel/watts/examples/1App_OpenMC_VHTR"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think providing this link should be necessary! the Python script should know the path to this input already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed workflow_dir

@@ -0,0 +1,86 @@
#!/bin/bash
########################################################################
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how this file does everything for you. Great job setting it up!

@@ -0,0 +1,8 @@
watts{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to develop additional templates - for variables, for plugin, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Developed templates for variables, plugins, workflow_level1, etc.

# chmod 777 watts_ui.py
# execute with command: `python watts_ui.py -i examples/watts_comprehensive.son`
###

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these comments can be removed now!

MaxOccurs=NoLimit
}
postprocessors{
Description = "[optional] postprocessor "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment out

}
}
workflow_dir{
Description = "[Required] Workflow directory: Dir where all files are located in. Necessary if have multiple input files."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this option is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removedworkflow_dir

else:
watts_params, app_result = run_direct(
watts_params, watts_plugins[plugin_ID])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the run_direct - there may be ambiguity on the orders of the different plugins. I am not sure Workbench will return you the plugin in a given order. We may ask a user to provide some ordering for the workflow to clarify the order the different plugins needs execution such as: plugin(1)=ID1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code now determines the sequence of plugin based on the order. For example, if we have

plugin = ID1
plugin = ID2

The code will run ID1 first then ID2.

show_stdout=plugin['show_stdout'],
show_stderr=plugin['show_stderr'])

elif plugin['code'].upper() == 'OPENMC':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to provide documentation in WATTS developers on how they can add their plugin information to watts_ui. This needs to be done in schema and here (at minimum). I recommend providing this in contributing.rst file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated contributing.rst to include instructions on how to add new plugin to watts_ui.py and watts.sch


# Store output data from app_result to watts_params
# Special treatment for OpenMC
if plugin['code'].upper() == 'OPENMC':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be moved above to have a 'post-run' logic added in plugin specification? I will need to do the same for PyARC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided to disregard this in the last meeting.

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't done a full review yet but I do have two high-level design questions/comments:

  1. It seems to me that much of the code in watts_ui belongs in workbench rather in watts. Would it make more sense to try to get this code merged upstream in workbench?
  2. As is, it seems like this functionality only works if you have cloned/downloaded the watts repo. For someone who just runs pip install watts, there's no way they would have access to this functionality.

doc/source/user/workbench.rst Outdated Show resolved Hide resolved
doc/source/user/workbench.rst Outdated Show resolved Hide resolved
doc/source/user/workbench.rst Outdated Show resolved Hide resolved
doc/source/user/workbench.rst Outdated Show resolved Hide resolved
@zhieejhia93
Copy link
Contributor Author

@paulromano Thanks for the comments.

@nstauff Do you think we should have a meeting to talk about Paul's suggestion on getting the code merged upstream in Workbench? Thanks.

@nstauff
Copy link
Contributor

nstauff commented Jan 13, 2023

Thanks @paulromano - good comment that pip install won't enable this capability. I had not realized this.
@zhieejhia93 - Having watts_ui directly developed within Workbench could be possible I think (and beneficial as we can streamline all the installation steps) - we need to have a meeting with Rob Lefebvre to discuss it. We would need however to setup a new open-source license for watts_ui.

fi

mkdir bin
ln -s $workbench_path/bin/sonvalidxml bin/sonvalidxml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will crash if we already have bin there - which will be the case if we are updating workbench install.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allowed transfer to skip if bin already exists

printf "${RED}ERROR: wasppy already exists in $current_path.${NC}\n"
exit 1
fi
ln -s $workbench_path/wasppy ./
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will crash if we already have wasppy there - which will be the case if we are updating workbench install.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allowed transfer to skip ifwasppy already exists

@zhieejhia93
Copy link
Contributor Author

@nstauff There's one thing that I forgot to mention in our meeting. If you set up Workbench according to the instructions, there might be several examples cases that you will not be able to run. This is because currently we directly install WATTS into Workbench using pip, which has WATTS v0.4.0. Since then, some changes have been made to WATTS that supports an executable argument that allows executable to be specified directly. I didn't want to develop watts_ui.py based on an outdated version of WATTS so for some examples where executable is provided directly, you will run into errors if you try to run them.

As a temporary workaround, you can copy the files in the src/watts directory from WATTS to /Applications/Workbench-5.0.1.app/Contents/rte/conda/lib/python3.8/site-packages/watts and replace the existing files. This is a temporary workaround that is only intended for internal use and testing. Once the latest version of WATTS is available on pip this step will not be necessary anymore.

@nstauff
Copy link
Contributor

nstauff commented Jan 19, 2023

Thanks @zhieejhia93 ! Let's discuss at next week's meeting if we could get a new WATTS release soon to address this issue.

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.

3 participants