Removed FuzzyTM dependency from setup.py

[ERijck]

Removed the FuzzyTM dependency from setup.py. Fixes #3423.

[piskvorky]

This PR seems huge – didn't we want to just remove one line from setup.py?

[ERijck]

Sorry, I thought that's what I did. I will aim to do it again today.

[ERijck]

I'm not sure why, but it seems that previous commits are part of this PR too. Shall I close this PR and start a new one?

[osma]

@ERijck I think you are reusing an old branch flsamodel for this PR. It would be better to create a new branch instead, make the one-line commit there, and then open a PR.

[piskvorky]

That's weird. The previous commits should already be a part of develop, from the (already merged) #3398. Github shouldn't show them in the diff.

@mpenkov did we revert that PR or something? Where did the commits like 6c7183e go 🤔

[osma]

Looking at this from the outside: I think the PR got merged (into develop) and rebased/squashed/whatever at the same time. Thus the commit IDs are different on the original PR branch flsamodel and the develop branch. The commit you mention doesn't exist on its own on the develop branch, but it's part of commit 45d35ee which was created by squashing all the commits on the PR branch into one, then merged into develop.

Normally you abandon (often delete it entirely) the PR branch after merging it to main/master/develop/whatever. It's not good practice to keep adding commits to a PR branch after it's been merged.

Anyway the solution is to create a new branch with just a single one-line commit.

[piskvorky]

That would explain it. Squashing is against our policy, so that's another omission – a double whammy :)

Anyway yes, a solution is to branch off develop and do the necessary change. I'm thinking presenting a more user-friendly error when the fuzzytm import fails is also worthwhile (similar to how we handle other optional dependencies).

Let's see if any tests fail after changing the setup.py. That might need adjusting as well.

[osma]

Squashing is against our policy, so that's another omission – a double whammy :)

It seems to me that all recent PR merges on the develop branch were done by squashing commits. Not just this one.

I'm thinking presenting a more user-friendly error when the fuzzytm import fails is also worthwhile (similar to how we handle other optional dependencies).

There is no fuzzytm import in the current code base, so I don't see how it could fail...

[ERijck]

Sorry, I am relatively new to the git workflow and still learning. I have created a new branch and added, committed and pushed the new setup.py (without fuzzytm dependency) file. However, the new PR still shows previous commits. This must be the squashing you are referring to. Is it correct that I should remove previous commits from develop and then only add/commit the removal of fuzzytm dependency?

[osma]

@ERijck

Switch to your local develop branch:

git checkout develop

Make sure your local develop branch is in sync with the origin develop branch (e.g. do a git pull origin develop).

Then create a new branch as a fork of the develop branch:

git checkout -b remove-fuzzytm-dependency

Edit setup.py, commit, push.

Alternatively, you can do this directly from the GitHub UI. Navigate to setup.py, click the edit button (pencil icon), make the change, commit and save it as a new pull request.

[piskvorky]

It seems to me that all recent PR merges on the develop branch were done by squashing commits. Not just this one.

@mpenkov did I miss something, are we squashing PRs now?

I'm personally not a fan of rewriting history (and not only for the reasons manifested in this ticket). I'd prefer normal merges – do you remember the reasoning for this change?

[mpenkov]

I think we've been squashing as far as I can remember.

In general, I agree with you, that rewriting history is bad. In the case of PRs, I don't think it counts as "rewriting history" (or at least, is not bad) because:

1. the commits in the PR are not part of the history of the branch they are being merged into (often, they are not even part of the repo, because they are coming from the contributor's fork)
2. the branch containing the PR gets destroyed after the merge (in vast majority of cases) so there is no inconsistency (with the exception of what happened in this PR, but that appears to be user error)

To give an example of the "bad" kind of rewriting history: squashing commits on "develop" and then force pushing to upstream. That leaves people out of sync with upstream, and requires effort on their part to work out what happens and how to get back in sync again.

Squashing can also be good in some cases. It's agnostic to the granularity of commits in the PR. Some people commit often, some less so. From our point of view, we squash their contribution to a single commit, it's always just one thing. Have a look at how e.g. #3427 was authored. It contains half a dozen commits to change one line. Do we want them all as individual commits in our history?

As you can see I have a mild preference to squashing PRs when merging, but it's up to you. Let me know if you do want me to switch to merge commits in the future.

[ERijck]

I have precisely followed @osma's advised steps (as described below). For this, I have created a new pull request #3437. Still, all these previous commits are there.

@ERijck

Switch to your local develop branch:

git checkout develop

Make sure your local develop branch is in sync with the origin develop branch (e.g. do a git pull origin develop).

Then create a new branch as a fork of the develop branch:

git checkout -b remove-fuzzytm-dependency

Edit setup.py, commit, push.

Alternatively, you can do this directly from the GitHub UI. Navigate to setup.py, click the edit button (pencil icon), make the change, commit and save it as a new pull request.

[piskvorky]

Commits are also a measure of past effort (of the PR, of the project). As well as a measure of authorship and author footprint & effort.

And then there's git bisecting to locate errors; informative commit messages (hopefully) related to a concrete commit = set of changes etc.

Btw commits are never "a part of the history of the branch they are being merged into"… until they're merged :) Not sure what you meant.

In any case, the path here is clear:

1. Remove the overlooked dependency.
2. Check that all tests pass, and that any import errors introduced are meaningful. For example, I see some pyfume import in flsamodel.py – what will happen to it when fuzzytm is gone?
3. Release a 4.3.1 bugfix.

@ERijck if you follow @osma's steps above, you should get a branch with a single extra commit in it. This PR has 7 commits in it, which doesn't sound right. What exactly did you do?

[osma]

@ERijck I suspect that your local develop branch may not be in sync with the origin develop branch. It may already have the commits from your older PR branch. Thus when you fork a new branch off of your local develop branch, it already comes with some "extra" commits.

There are many ways to fix this but perhaps the easiest (even if it's a bit drastic) is to delete your local develop branch and then pull it again from origin.

[ERijck]

Finally, it worked. I will add an ImportError and write documentation explicitly mentioning how to install pyfume.

Thanks for the help and patience @piskvorky, @osma and @mpenkov!