search

watertap-org / watertap

The WaterTAP development repository
https://watertap.readthedocs.io/en/latest
Other
59 stars 57 forks source link

Seemingly random MacOS failure in test_cstr #1473

Open adam-a-a opened 1 month ago

adam-a-a commented 1 month ago

I have seen the following glitch show up in MacOS checks which I can resolve by just rerunning the check. Just seeing this for the second time on the same file (test_cstr) so reporting it in case it pops up again.

image

lbianchi-lbl commented 1 month ago

Zooming out a bit, we should consider dropping support for (i.e. stop testing in CI) the IDAES-less macOS environment. It's a non-negligible maintenance burden and we might decide it's not worth it.

See also: #1306

adam-a-a commented 1 month ago

Noticed this failure on #1471

lbianchi-lbl commented 2 weeks ago

Zooming out a bit, we should consider dropping support for (i.e. stop testing in CI) the IDAES-less macOS environment. It's a non-negligible maintenance burden and we might decide it's not worth it.

See also: #1306

This (i.e. replacing our "bootleg" macOS CI environment with Apple Silicon GHA runners using IDAES solvers) is being done in #1483.

lbianchi-lbl commented 1 week ago
lbianchi-lbl commented 1 week ago

The macOS CI check is indeed failing in the same way in #1486, which would confirm @adam-a-a's hypothesis/observation that the failure is possibly related to a change in IDAES between versions 2.5.0 and 2.6.0rc0: https://github.com/watertap-org/watertap/actions/runs/10747258986/job/29809470692

image

adam-a-a commented 4 days ago

@lbianchi-lbl this same test exists on IDAES for the same unit. The only difference is in the WaterTAP version of this unit, we add a variable and simple constraint. That is, we add hydraulic retention time as a variable and the associated constraint for it, relating retention time to reactor volume and inlet flowrate.

I don't know if this cstr test may have had a similar macOS test failure on the IDAES repo.

lbianchi-lbl commented 4 days ago

@lbianchi-lbl this same test exists on IDAES for the same unit. The only difference is in the WaterTAP version of this unit, we add a variable and simple constraint. That is, we add hydraulic retention time as a variable and the associated constraint for it, relating retention time to reactor volume and inlet flowrate.

I don't know if this cstr test may have had a similar macOS test failure on the IDAES repo.

Thanks for the extra info @adam-a-a. On the IDAES side, I don't believe we have any CI job running without the IDAES solver, so it's possible (but agani, not proven either way) that the IDAES/idaes-pse counterpart of this test would fail in the same way if it was run in the same environment (i.e. non-IDAES Ipopt, etc).

andrewlee94 commented 3 days ago

@adam-a-a Would you be able to run the diagnostics tests (both structural and numeric) and paste the output here? This would help us try to work out what the root cause might be.

Edit: It should not matter which platform you run this on, as the underlying issues will likely be the same.

adam-a-a commented 3 days ago

I could do so but currently depend on the GH check for Mac to see results.

andrewlee94 commented 3 days ago

@adam-a-a Running it on your local machine should be good enough. If there is a modeling issue, it will likely show up no matter what platform you run on; structural issues depend only on the model, and even numerical issues will likely be apparent in the before and after solve runs unless you are really unlucky.

andrewlee94 commented 3 days ago

@adam-a-a Also, I note you said this could be fixed by re-running the test - is that correct? If so, we are dealing with something more complicated (and something that I am not sure how to deal with).

adam-a-a commented 3 days ago

@adam-a-a Also, I note you said this could be fixed by re-running the test - is that correct? If so, we are dealing with something more complicated (and something that I am not sure how to deal with).

@andrewlee94 I am not certain that this is the case at the moment but can check again.