wavefunction91
Add downstream integration tests
As was made apparent in #103, sometimes we need to make large changes, but checking that those changes down break things down stream is a bit challenging. We should add unit tests to make sure that we can (at least) compile GauXC PRs relative to downstream projects to alert of potential problems without loud "knocking" every time we do something sweeping.
The first thing would be to define a set of downstream projects and to document them. Right now, the primary downstream integrations we need to be concerned about are
- [ ] NWChemEx (@wavefunction91,@ryanmrichard)
- [ ] MPQC (@evaleev)
- [ ] ExaChem (@ajaypanyala @dmejiar)
- [ ] ChronusQ (@elambros)
- [ ] Psi4 (@loriab @davpoolechem)
Happy to have suggestions for how we might want to go about this.
TL;DR the tests I think you are proposing are usually done by the consumer, not the provider.
Slightly longer answer. Assuming GauXC never rewrites git history, then consumers' build systems and CI/CD pipelines should rely on pinned commits/versions of GauXC to avoid this problem. If the consumer also wants to be notified when changes break compatibility, they should create a nightly test that uses the newest version of GauXC (usually nightly so you don't have to watch for upstream changes).
Longest answer. As I understand this @wavefunction91 wants to have integration tests which run as part of GauXC's CI/CD. These integration tests will build downstream consumers as part of the PR process. The failure of an integration test will then alert @wavefunction91 to the fact that an API-breaking change has occurred. While an admirable goal, it also doesn't scale. This is because it is going to couple GauXC's CI/CD to the build system of each downstream consumer. The problem with such a design is that you're now creating a feedback loop where by if a downstream consumer decides to implement breaking changes in either their build system (notably adding new dependencies) or in how they use GauXC, then such changes may end up breaking the GauXC test meant to ensure that GauXC doesn't break the downstream repo (and things get real bad if the downstream repo adds an integration test to ensure that such changes don't break GauXC). AFAIK, in the wild, these sorts of integration tests are usually only done for the target consumer, or for consumers whose use is considered a feature (e.g., many projects ensure they don't break FAANG products so they can boast about being used by them).
@ryanmrichard These are all good points.
A few clarifications:
- I agree that downstream users should pin GauXC revisions. However, there are a number of projects that we integrate with that we want (to gently nudge?) to make sure that we're always using the leading edge (or at least compatible with it). That, and for better or worse, I'm currently the maintainer of several of the integrations - something like this would more be for my own sanity than anything else (with the added bonus that downstream maintainers can be reasonably certain that, even if they do pin, they can bump with confidence).
- I think maybe my inclusion of this into the CI/CD pipleline may have given the impression that I would add it as a blocker (HT @loriab for offline discussion) - that's not the case. Adding an action to check the integrations would just give us and the downstream devs a heads up "hey, you should look at this", rather than "I'm going to hold up the pipeline to make sure that this is fine". This also tells downstream users whether or not a particular commit is safe to use with a particular version of a downstream integration (I get the question of "why doesn't this commit work with this code" reasonably often, this would give us something to point to).
- I generally agree that such an action should live downstream, in principle. And if we can get folks to do that, it would be great, I'm all for converting this issue into "make sure downstream folks that care have things in place to make sure catastrophic things don't happen". This was more meant to start a discussion (which it has!) than to roll out a single true path forward - I'm very flexible
The points about downstream repos potentially introducing breaking changes are good ones, and would definitely be more in favor of consumer-side testing. Most (all?) of the (potentially) interested parties are pinged here, let me know how you all want to handle this. I'm pretty open with the caveat that I don't really want to be manually checking PRs downstream anymore.
@wavefunction91 gotcha. If I were you I'd bite the bullet and write the integration tests for each of the interfaces you personally maintain in the respective downstream packages (assuming you have developer access). My rationale being that downstream development is likely to continue to occur independently of what you do here in GauXC. Besides breaking changes being added to GauXC, it's also not inconceivable that breaks can occur as a result of changes in the downstream repo. If that happens, and the tests are in the GauXC repo, then while you will know that the changes break GauXC compatibility, unless the downstream developers are monitoring your tests, they're probably still going to be blindsided by the break. There's also the slight glimmer of hope, that by having the downstream repo's CI fail, the downstream developers may look into the problem themselves and not just immediately ping you...
FTR, I volunteer @jwaldrop to maintain the integration test on the NWChemEx side.