How to proceed with PR Gate that runs notebooks against the last released Fairlearn version?

[romanlutz]

This keeps causing confusion, e.g. most recently #434 (comment)

Any suggestions for how to rename the pipeline? This is complex because in general it should be green/passing. If we make the conscious decision to break something compared to the last version it gets red. From that point onward it stays red until the next release. So we definitely do want to be alerted when it goes red for the first time. That's what makes this tricky.

@adrinjalali suggested in the past that things like this could perhaps be fixed just before a release. [I hope I'm not rephrasing his ideas in a way that misrepresent them...]

So I'm curious how people feel about this. In a way I feel like solving the documentation versioning in the way scikit-learn does it, i.e. the webpage can be viewed based on whatever version of the package you're using incl. notebook, and therefore you will have no trouble finding the compatible notebook. That would perhaps address the issue sufficiently.

FYI @kevinrobinson helped out some folks over the past week or so who raised this issue here on GitHub, so perhaps you also have thoughts.

FYI @koaning who faced this issue as well.

FYI @riedgar-ms @MiroDudik

[riedgar-ms]

I'd say that there's no value to a build which is perpetually red, and hence ignored. Fundamentally, we need to get better about compatibility (which is what my PR over in fairlearn-proposals is about). Of course, the build is currently checking forwards compatibility, which is necessarily going to break as features are added (although I believe that the current issues are simply due to naming instability). To keep this build green through such changes, we would need to have tests and notebooks declare their 'minimum supported version.'

Having the documentation versioned is related, and very good to have, but doesn't solve the issue that we have too many breaking changes too often.

Until we get to that v0.5.0 compatibility commitment, it might be worth putting '[Optional]' or something like that on the build name. That will at least reduce the confusion.

[adrinjalali]

I still don't think there should be a CI step which tests against the latest released version, that's not a sensible thing to me really.

We do need to have different versions of the docs online, since API changes always happen and people should see the latest released version online and not what's on master (unless they explicitly go to the latest version of the docs which is built from the master).

And checking for backward compatibility should be clear from checking the tests and running the docs on master itself. If a PR has to change anything in the docs, then it's a breaking change.

Also we don't really need the notebooks to test that, as long as we have good docs and examples which use the API, we'll notice the breaking changes.

[riedgar-ms]

And checking for backward compatibility should be clear from checking the tests and running the docs on master itself.

Do you mean keeping backwards-compatibility tests in master, @adrinjalali ? I had been thinking that we would have builds which would take tests from supported releases, and run them against Fairlearn installed from master.

[romanlutz]

@adrinjalali that sounds reasonable to me. I’ll see whether I can carve out some time for doc versioning. Last time I looked at scikit-learn’s logic for that it looked very complicated.

[adrinjalali]

I don't mean putting anything more than what we already have in master @riedgar-ms . When reviewing a PR, if the PR has to change anything in the tests or the docs to make the CI pass, that means it's probably not backward compatible, and that's enough, isn't it?

[riedgar-ms]

@adrinjalali Agreed. But I do like to have builds which double check things :-)

[adrinjalali]

What I'm trying to say is that it's the wrong check. Certain changes to master break things and that's okay as far as we document it and/or warn the user. master's not supposed to pass all the tests with an outdated (aka released) version.

[riedgar-ms]

If we are trying to provide some level of backwards compatibility, sure master should be able to run older tests (within the compatibility window)?

[romanlutz]

@riedgar-ms I am not even sure that makes sense. There are tests that use things which aren’t around anymore. For example, the current pipeline is failing because a member name was changed from to . That isn’t really a breaking change in some ways because it was underscored to begin with.
What you’re proposing is in essence a different version (different in scope of tests) of the existing pipeline, right? Perhaps I’m missing something.

I certainly see the point in being intentional about documenting breaking changes better for users. In part @kevinrobinson ’s PR will help with that since we get a dedicated place to document how to upgrade from version to version.

That along with versioned docs should go a long way. I’m not opposed to further checks in principle, but they may be better off not being PR checks.

[riedgar-ms]

I know that there will be breaking changes in future, but if we keep having breaking changes for every single release on PyPI, we are neither going to get nor keep users. At some point, we're going to have to start making the promise that code that works with x.y.z will also work with x.y.z+1. Running the tests of an older version (but within the compatibility window) against master is a way of verifying that compatibility.