gmlc-dispatches / dispatches

Primary repository for distributed dispatches software tools
https://dispatches.readthedocs.io/
Other
12 stars 34 forks source link

Add CI workflow to test RAVEN integration #157

Closed lbianchi-lbl closed 1 year ago

lbianchi-lbl commented 1 year ago

Resolves: #76

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.md and COPYRIGHT.md file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.
lbianchi-lbl commented 1 year ago

@joshua-cogliati-inl I've added the following step to clone the RAVEN repository and run one of the examples as suggested by @radhakrishnatg: https://github.com/gmlc-dispatches/dispatches/blob/ad82bd1242fe498abbdfa1b1ace60c0457ff4038/.github/workflows/raven-integration.yml#L51-L60

However, this seems to open a GUI window that must be closed manually for the process to terminate, which makes it hard to use this in CI. Is there a way to run RAVEN in "batch mode", so that the raven_framework command terminates without manual intervention? I've tried raven_framework --help and the website to see if I could find something like this, but without luck.

joshua-cogliati-inl commented 1 year ago

I would recommend a different test (doc/workshop/forwardSampling/exercises/1_sample_and_plot.xml plots to screen) such as tests/framework/user_guide/ForwardSamplingStrategies/forwardSamplingGrid.xml

GabrielSoto-INL commented 1 year ago

if additional (simple) TEAL tests are of interest, I would recommend TEAL/tests/Cash_Flow_input_NPV.xml. an additional one that might pertain more to #150 would be TEAL/tests/PyomoTest.py (both found here)

joshua-cogliati-inl commented 1 year ago

@lbianchi-lbl

 python -c "
  from raven.ravenframework.utils import xmlUtils; print(xmlUtils)
  from raven.scripts import externalROMLoader; print(externalROMLoader)
  "

This should probably be:

 python -c "
  from ravenframework.utils import xmlUtils; print(xmlUtils)
  from ravenframework import ROMExternal; print(ROMExternal)
  "

If there is code still using externalROMLoader let me know and I can suggest how to modify it to switch to ROMExternal.
The pip package installs RAVEN into ravenframework, not raven (There was already a different raven in pip)

Thanks.

lbianchi-lbl commented 1 year ago

If there is code still using externalROMLoader let me know and I can suggest how to modify it to switch to ROMExternal. The pip package installs RAVEN into ravenframework, not raven (There was already a different raven in pip)

Thanks for looking into this @joshua-cogliati-inl. For the import checks, my approach was to look at #150 and collect any RAVEN/TEAL imports used there. In particular, the externalROMLoader is taken from:

https://github.com/gmlc-dispatches/dispatches/blob/9532ad2a8bdc9eb35546e4ef52cbe60ffd1fa705/dispatches/case_studies/nuclear_case/SynHist_integration.py#L33

joshua-cogliati-inl commented 1 year ago

https://github.com/gmlc-dispatches/dispatches/blob/9532ad2a8bdc9eb35546e4ef52cbe60ffd1fa705/dispatches/case_studies/nuclear_case/SynHist_integration.py#L33

Thanks, I left some review comments over at that pull request.

joshua-cogliati-inl commented 1 year ago

We released raven-framework 2.2 , so can you switch requirements-raven.txt to use that instead of the release candidate? I think that should fix at least one of the failures.

lbianchi-lbl commented 1 year ago

We released raven-framework 2.2 , so can you switch requirements-raven.txt to use that instead of the release candidate? I think that should fix at least one of the failures.

@joshua-cogliati-inl thanks, I was just about to post that https://github.com/idaholab/raven/commits/devel/ravenframework/ROMExternal.py seems to have been added recently and therefore was probably not included in the code version that was being used here. I'll update the requirements with the non-RC version.

codecov[bot] commented 1 year ago

Codecov Report

Base: 93.69% // Head: 93.69% // Decreases project coverage by -0.00% :warning:

Coverage data is based on head (91ff131) compared to base (bcbaba8). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #157 +/- ## ========================================== - Coverage 93.69% 93.69% -0.01% ========================================== Files 59 59 Lines 7105 7104 -1 ========================================== - Hits 6657 6656 -1 Misses 448 448 ``` | [Impacted Files](https://codecov.io/gh/gmlc-dispatches/dispatches/pull/157?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gmlc-dispatches) | Coverage Δ | | |---|---|---| | [...a\_supercritical\_plant/tests/test\_usc\_powerplant.py](https://codecov.io/gh/gmlc-dispatches/dispatches/pull/157/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gmlc-dispatches#diff-ZGlzcGF0Y2hlcy9jYXNlX3N0dWRpZXMvZm9zc2lsX2Nhc2UvdWx0cmFfc3VwZXJjcml0aWNhbF9wbGFudC90ZXN0cy90ZXN0X3VzY19wb3dlcnBsYW50LnB5) | `100.00% <ø> (ø)` | | | [...ischarge\_design\_ultra\_supercritical\_power\_plant.py](https://codecov.io/gh/gmlc-dispatches/dispatches/pull/157/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gmlc-dispatches#diff-ZGlzcGF0Y2hlcy9jYXNlX3N0dWRpZXMvZm9zc2lsX2Nhc2UvdWx0cmFfc3VwZXJjcml0aWNhbF9wbGFudC9zdG9yYWdlL2Rpc2NoYXJnZV9kZXNpZ25fdWx0cmFfc3VwZXJjcml0aWNhbF9wb3dlcl9wbGFudC5weQ==) | `95.26% <100.00%> (ø)` | | | [...ant/storage/tests/test\_discharge\_usc\_powerplant.py](https://codecov.io/gh/gmlc-dispatches/dispatches/pull/157/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gmlc-dispatches#diff-ZGlzcGF0Y2hlcy9jYXNlX3N0dWRpZXMvZm9zc2lsX2Nhc2UvdWx0cmFfc3VwZXJjcml0aWNhbF9wbGFudC9zdG9yYWdlL3Rlc3RzL3Rlc3RfZGlzY2hhcmdlX3VzY19wb3dlcnBsYW50LnB5) | `100.00% <100.00%> (ø)` | | | [dispatches/unit\_models/elec\_splitter.py](https://codecov.io/gh/gmlc-dispatches/dispatches/pull/157/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gmlc-dispatches#diff-ZGlzcGF0Y2hlcy91bml0X21vZGVscy9lbGVjX3NwbGl0dGVyLnB5) | `95.79% <100.00%> (-0.04%)` | :arrow_down: | | [dispatches/unit\_models/tests/test\_concrete\_tes.py](https://codecov.io/gh/gmlc-dispatches/dispatches/pull/157/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gmlc-dispatches#diff-ZGlzcGF0Y2hlcy91bml0X21vZGVscy90ZXN0cy90ZXN0X2NvbmNyZXRlX3Rlcy5weQ==) | `83.53% <100.00%> (ø)` | | | [...ches/unit\_models/tests/test\_heat\_exchanger\_tube.py](https://codecov.io/gh/gmlc-dispatches/dispatches/pull/157/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gmlc-dispatches#diff-ZGlzcGF0Y2hlcy91bml0X21vZGVscy90ZXN0cy90ZXN0X2hlYXRfZXhjaGFuZ2VyX3R1YmUucHk=) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gmlc-dispatches). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gmlc-dispatches)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

lbianchi-lbl commented 1 year ago

The notebook execution checks are failing because of #167, but I think we should merge this anyway since the failures are not related to the content of this PR.