IDAES / idaes-pse

The IDAES Process Systems Engineering Framework
https://idaes-pse.readthedocs.io/
Other
216 stars 234 forks source link

Backwards Compatibility Promise and Tests #1037

Closed andrewlee94 closed 1 year ago

andrewlee94 commented 1 year ago

With the transition to IDAES v2.0, we need to add a formal definition of what we consider backward compatibility and a set of tests that will verify this.

eslickj commented 1 year ago

This is probably my stupidly simple thinking, but can we just run the IDAES 2.0 tests with all new versions on IDAES 2.x. If those tests don't break we can consider it backward compatible. I assume we will want backward compatibility until we get to 3.0?

lbianchi-lbl commented 1 year ago

This is probably my stupidly simple thinking, but can we just run the IDAES 2.0 tests with all new versions on IDAES 2.x. If those tests don't break we can consider it backward compatible. I assume we will want backward compatibility until we get to 3.0?

Actually, from my experience trying to do exactly this for the v1 -> v2 compatibility, I think this is a viable approach that we should be able to implement without too much effort (basically mostly to ensure that our test suite is "cleanly extractable" so that it can be run reliably against a separate installation of the idaes package).

jsiirola commented 1 year ago

The problem with "just running the 2.0 tests" is that we should probably be judicious about picking which 2.0 tests to run. The testing infrastructure often is fragile, and can fail for "non user facing changes". For example, when a test fails that compares expression structures or the variable orderings: is that really a backwards incompatibility?

At least for Pyomo it has been helpful to have a separate "user-centric" test suite with a reasonably generous definition of acceptance. For example, when comparing output, we don't fail if deprecation warnings appear. We also design these to be more like regression tests (many are, "did the code run". If the problem is well enough behaved, we might attempt to solve it and compare the final objective. Now in fairness, this is easier because the "test suite" is just all the code examples from the Book. I think we could do something similar for IDAES by basing the backwards compatibility suite on the set of tutorials.

eslickj commented 1 year ago

Being very lazy, we could take the approach that if one of the fragile tests fail, we worry about whether to kick it out at that time.

If backward compatibility doesn't mean the results stay the same, maybe we could just run unit tests. That should at least mean the API doesn't break, but I'm not sure how good the coverage is if we are limited to unit tests.

andrewlee94 commented 1 year ago

@eslickj That is hardly a promise of backward compatibility however.

eslickj commented 1 year ago

@andrewlee94, it's just a more lazy version of what @jsiirola suggested. If we say the test broke because it was ridiculously fradgile or whether we kick out a bunch of tests from the start, what's the difference?

andrewlee94 commented 1 year ago

@eslickj Functionally not much, but it appears far better if we make the decision at the start and then set the tests in stone.

We could consider basing the backward compatibility tests around the current tutorials and the current unit tests. The core code at least is well covered by just unit tests, with only initialization being missing (and we intend to rebuild that anyway). Unit tests should not involve any solvers, which gets around some of the big issues, but they do occasionally look at the expected form of expressions.

Robbybp commented 1 year ago

Following up from the discussion at the dev call today, I think a (relatively) small set of user-oriented tests that we strictly maintain compatibility with is a really good idea. As was mentioned, I think the tutorials are the best place to start. Ideally these tests would contain the examples as well, but some of our examples may be more fragile than we would like for these tests.

I also think that these tests should test the result of solves, with a requirement that these solves are "stable".

Separately, I believe that there should be a suite of tests for "critical property packages" that assert that they perform the "right calculations" over a range of conditions, similar to what I implemented for the CLC oxidation property packages.

andrewlee94 commented 1 year ago

A new repo has been set up to contain the backward compatibility and related tests: https://github.com/IDAES/idaes-compatibility

An initial set of tests based on the tutorials and examples, as well as some verification of Helmholtz EoS models have been added, so all that is left is to set up a CI runner for those.