Skip to content

Proposal for Backwards Compatibility & Releases #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Proposal for Backwards Compatibility & Releases #11

wants to merge 17 commits into from

Conversation

riedgar-ms
Copy link
Member

We have been very cavalier with backwards compatibility to date. This is a proposal to address that, and provide some more stability to our users, while recognising that we're still early-stage.

riedgar-ms added 3 commits May 7, 2020 14:27
Signed-off-by: Richard Edgar <[email protected]>
Signed-off-by: Richard Edgar <[email protected]>
Signed-off-by: Richard Edgar <[email protected]>
@riedgar-ms riedgar-ms requested review from romanlutz and MiroDudik May 7, 2020 20:05
## Proposed Solution

Starting with `v0.5.0` we should make a commitment that anything which works at `v0.n.m_0` will also work for `v0.n.m` so long as `m >= m_0`.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very reasonable. I like this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps something I should highlight in the text is that I'm not saying anything about n and n+1 for support. I am anticipating breakages there (although we would try to minimise them).

For full compliance with the above, we will have to devise a means of allowing each notebook to specify its `m_0`.
Our notebooks do not provide comprehensive coverage of Fairlearn, but enforcing a constraint like this will be a dramatic improvement over the current situation (particularly since new users will likely try our notebooks first).

In order to support less mature functionality, we should also add a `fairlearn.experimental` package (see [a similar namespace in SciKit-Learn](https://scikit-learn.org/stable/modules/classes.html#module-sklearn.experimental)).
Copy link
Member

@MiroDudik MiroDudik May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we need this yet. I guess I wonder how much overhead this would be on the testing side (in terms of coding this up and then maintaining it) and what we might put into this subpackage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not opposed to fairlearn.experimental in general, although I don't think it would have solved any of our issues. We're reworking all the core parts and that's why we're causing issues right now.

Scenarios for fairlearn.experimental in my view are:

  • new mitigation techniques that we're trying out (e.g. for research purposes) and that we don't necessarily want users to actively use unless they're aware of the caveats
  • let's say someone wants to replicate our charts in matplotlib, that may be very incomplete at first and may need some work to get in a state where there's enough functionality (but would be too much for a single PR)

This is specific to Python code, so this wouldn't be usable for the dashboard and adding new components, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, fairlearn.experimental certainly wouldn't have helped with our current refactorings. The idea is that going forward, it gives contributors somewhere to 'play' without getting stuck supporting their earlier poor choices (which are inevitable, since users always come up with ingenious ways of breaking things :-) ).

For the UX, I don't know remotely enough about TS to comment. Perhaps something similar could happen there.

I'd very much appreciate experiences from other open source projects here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TS does have namespaces, so I've added a note.

@riedgar-ms riedgar-ms requested a review from mesameki May 8, 2020 14:55
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good start to me. I would really focus much less on notebooks (if any), and if there's anything in the notebooks which is not in the tests, I'd move them to tests.

It'd also be nice to limit the line length for an easier review between different revisions as usual ;)

Before it, `v0.4.5` reworked a smaller subset of this functionality, which caused smaller breakages.
There are also further changes to Metrics planned, which may well cause further breaks (although these should be more minor).
All this is obviously undesirable from a user standpoint - these look like 'patch' level releases, but are actually breaking their code.
Concretely, we've had users install Fairlearn with `pip`, and then find themselves unable to run Notebooks from our GitHub project - not because the functionality was missing, but because it had been renamed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an issue which is fixed with the versioned documentation and not letting users rely on notebooks from the main repo.

Copy link
Member Author

@riedgar-ms riedgar-ms May 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a note to that effect - although I'm sure people will still manage to download the wrong version.

As for concentrating the testing on the notebooks, I was thinking of that from a 'first user impression' perspective. The notebooks are where we're most likely to hit trouble.

(Although as you note below, we are wanting to move from notebooks to examples and we could just run the tests instead)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the text to have the tests run, rather than just the notebooks.

Comment on lines +19 to +20
Starting with `v0.5.0` we should make a commitment that anything which works at `v0.n.m_0` will also work for `v0.n.m` so long as `m >= m_0`.
However, we do *not* guarantee compatibility between `n` and `n+1` in this scheme (although we would seek to minimise breakage).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what we have in sklearn as well.

Starting with `v0.5.0` we should make a commitment that anything which works at `v0.n.m_0` will also work for `v0.n.m` so long as `m >= m_0`.
However, we do *not* guarantee compatibility between `n` and `n+1` in this scheme (although we would seek to minimise breakage).

We will enforce this using builds which run the notebooks currently in the repository against versions of Fairlearn published on PyPI.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't we moving away from notebooks?

This also has to do with release workflow. Here's a proposal:

We could say 0.xx.y+1 is a patch or bug fix release to 0.xx.y.
The master branch is 0.xx+1.dev0, and therefore changes on master can be breaking backward compatibility.
Whenever we want to push a patch release out, we backport the PRs which we'd like to include in 0.xx.y+1 from the master, into the branch for the release.

This also implies that there is a branch for at least each major release, and minor releases are tags on at different times on that branch. The different versions of the documentation are also generated from those branches.

In my experience checking for backward compatibility in reviews is not hard, and if we get used to caring about it, then it'll be fine. In terms of testing, if we really want to test against a different release, we could run the tests against a different release, instead of running the notebooks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moving away from notebooks?

Yes.... I should at the very least rephrase to be about running things from the examples directory.

However, your point about just running the tests is also good. I may have been overly concerned about saving on CPU cycles.

@romanlutz and @MiroDudik , what do you think about the release and branching strategy @adrinjalali is proposing? It looks reasonable to me, but would be more overhead over what we're currently doing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the proposal, making it a lot closer to the procedure described by @adrinjalali above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re notebooks - yes! It's just not done yet. Perhaps we should prioritize it.

We're pre-1.0 so we can currently only mess with 0... The only way to distinguish major releases from minor ones is therefore this last level (patch level). Otherwise we'll jump from 0.5.0 to 0.6.0 to 0.7.0 rapidly. It's really impossible to tell for users whether 0.7.0 is a major change or not at that point, since everything is flat. So fort that reason I'm not a huge fan.

I agree with testing against releases, not just running notebooks. That was just a quick band aid.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Not a huge fan" of what exactly - there are a few things going on here. I think you're concerned that our breaking changes are going to result in us going through minor versions quickly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was confusing myself a little bit. I think it's important to have a consistent approach and this is one such approach. No objections.

@riedgar-ms riedgar-ms changed the title Proposal for Backwards Compatibility Proposal for Backwards Compatibility & Releases May 14, 2020
Signed-off-by: Richard Edgar <[email protected]>
@kevinrobinson
Copy link

These all seem like good ideas to me! My two cents from the outside would be to focus more on communicating about the project's status, scope and intentions. That might be as minimal as writing a paragraph or three bullets at the top of the README about what the team considers stable, and what it intends to focus on doing in the next 3-6 months. That might be more immediately valuable.

Similarly, I'd vote for removing code, guides and examples that are known to be incomplete, not representative of values in fairlearn/fairlearn#436, etc. That may help elevate the role of improving the documentation, user guides and examples which may be a part of goals that the team has for project growth and adoption. Helping redpeople update version numbers seems good, but adding a changelog can point out the big things. A more immediate problem might be improving first-time encounters with the project so as to avoid things like fairlearn/fairlearn#448.

@riedgar-ms
Copy link
Member Author

@kevinrobinson , can you explain what you mean by "adding a changelog can point out the big things?" We already have a Changes.md file; do you mean something else?

Thank you for hopping in on fairlearn/fairlearn#448 (memo to self: look at the issue list more frequently). We have a build as part of the PR gate which highlights those breaks, since we've been caught by that before. I'm not sure if it was green for a full week after I put out v0.4.6, though :-(

@kevinrobinson
Copy link

@riedgar-ms sure, no problem!

re: changelog yep, having a CHANGES.md seems great! In the scenario where a user is thinking "I am using this library, and want to understand what will break, and how I need to migrate when I upgrade," that may involve flipping the perspective a bit. In other words, if you're trying to support users in understanding the stability and changes in the library, it may help to revise the changelog away from a developer-centric list of work that has been done, and more towards a user-centric communication about what has changed, why it has changed, and what they can do about it.

@riedgar-ms
Copy link
Member Author

Understood. We do have careful instructions for migrating from v0.2 (which had an even smaller userbase), but we haven't done that since. Your point that we should be doing so is a good one!

Signed-off-by: Richard Edgar <[email protected]>
@riedgar-ms
Copy link
Member Author

@kevinrobinson , have updated with a note about making sure Changes.md is useful for users and not just developers. Specifically by including migration information for breaking changes.

@romanlutz
Copy link
Member

Understood. We do have careful instructions for migrating from v0.2 (which had an even smaller userbase), but we haven't done that since. Your point that we should be doing so is a good one!

Taking a step back, I think CHANGES.md has its place that makes sense. I don't mind additionally adding context on how to upgrade as @kevinrobinson describes. I'm not sure whether this should be mixed because it may make the document less useful for both developers and users.

@riedgar-ms
Copy link
Member Author

Taking a step back, I think CHANGES.md has its place that makes sense. I don't mind additionally adding context on how to upgrade as @kevinrobinson describes. I'm not sure whether this should be mixed because it may make the document less useful for both developers and users.

Do you mean a Migration.md (say) which would be pointed to from the Changes.md @romanlutz ? I agree that the migration instructions could get long for the Changes.md - although that would be an incentive to avoid breaking changes :-)

@romanlutz
Copy link
Member

Taking a step back, I think CHANGES.md has its place that makes sense. I don't mind additionally adding context on how to upgrade as @kevinrobinson describes. I'm not sure whether this should be mixed because it may make the document less useful for both developers and users.

Do you mean a Migration.md (say) which would be pointed to from the Changes.md @romanlutz ? I agree that the migration instructions could get long for the Changes.md - although that would be an incentive to avoid breaking changes :-)

Rather a ReST file that’s part of the webpage. It can have upgrade instructions for every version. We can link to it from anywhere we want.

@adrinjalali
Copy link
Member

having a detailed changelog and a highlights post which could also include how to migrate separately is usually appreciated by users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants