Skip to content
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

Set minimum and maximum Nengo versions. #1001

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

Conversation

jgosmann
Copy link
Collaborator

Current master branch requires at least Nengo 2.8 because it is importing

from nengo.utils.compat import escape

This PR sets Nengo requirement in the setup file accordingly.

@jgosmann
Copy link
Collaborator Author

(reported here)

@arvoelke
Copy link
Contributor

arvoelke commented May 6, 2019

Note nengo.utils.compat was deprecated in Nengo version 3.0.0-dev and is scheduled to be removed upon the next minor release thereafter (e.g., 3.0.1 3.1.0).

@jgosmann
Copy link
Collaborator Author

jgosmann commented May 6, 2019

Note nengo.utils.compat was deprecated in Nengo version 3.0.0-dev and is scheduled to be removed upon the next minor release thereafter (e.g., 3.0.1).

I thought Nengo follows semantic versioning (mostly)? The removal is definitely a breaking change and should not happen in a minor increment and definitely not in a patch increment. (So the proper path to me seems to make another minor 2.x release with the deprecation and then remove it in 3.0.)

But apart from that, we could also set a maximum Nengo version or move the relevant imports to nengo-gui.

@arvoelke
Copy link
Contributor

arvoelke commented May 6, 2019

Sorry by minor release I meant e.g., 3.1.0. Not a patch increment. Details here under "deprecated". Does this seem reasonable?

@jgosmann
Copy link
Collaborator Author

jgosmann commented May 7, 2019

Depends on what meaning you assign to the version numbers. But removing it in a minor version contradicts Semantic Versioning which Nengo is supposedly using: https://www.nengo.ai/governance.html

@arvoelke
Copy link
Contributor

arvoelke commented May 7, 2019

Yeah, so you are technically right, but at the time the team decided that it's not worth following the semver spec exactly for cases such as these. Since the dev branch is already at 3.0.0-dev due to backwards-incompatible changes, and will be released with the deprecation notice, this would require us to hang on to this untested / dead code (note we are no longer testing or supporting Python < 3) for an entire major version, or release a version 4 soon after just to get rid of this. Current use of nengo.utils.compat should be pretty sparse and limited to the Nengo ecosystem and so we don't see it as a serious concern.

@jgosmann
Copy link
Collaborator Author

jgosmann commented May 8, 2019

or release a version 4 soon after just to get rid of this

What is the problem with that? The way I see it is that the purpose of Semantic Versioning is to give you a reliable contract that you can depend on to specify version ranges for dependencies. If you are worried how it looks to the user or want to decide on version increments by how "big" or impactful the changes are, that's fine too, but maybe it shouldn't say that Nengo is using Semantic Versioning?

Or I suppose I could see an argument that nengo.utils.compat is not considered part of the public API (and Semantic Versioning only applies to that part of the API). However, in that case, why not just remove it without the deprecation step?

Also, it is much more critical if you intend to remove the deprecated SPA module and network inputs in a minor release because these are part of the public API.

@tbekolay
Copy link
Member

tbekolay commented May 8, 2019

nengo.utils.compat is not considered part of the public API (and Semantic Versioning only applies to that part of the API)

I would agree with this, I don't consider the things in nengo.utils to be part of the public API. The deprecation step is mostly a convenience for downstream Nengo projects so we don't have to necessarily update them all before making the 3.0.0 release.

@arvoelke
Copy link
Contributor

arvoelke commented May 8, 2019

What is the problem with that?

IMO it would make the versioning less meaningful if the only difference between 3 and 4 was removing an internal API that was only kept around to help smoothen the transitioning our own projects. Meanwhile the difference between version 2 and 3 is quite enormous, spanning multiple years of development.

However, in that case, why not just remove it without the deprecation step?

Mostly as a courtesy / convenience for ourselves.

I'll also just point out that these sorts of decisions to deviate from the semantic versioning spec are fairly common in some of the most popular / depended upon packages that claim to use semantic versioning. For example, pytest frequently removes deprecated features even on patches. IMO it's about finding the right balance between all these considerations, and sometimes this doesn't strictly follow the letter of the law.

@jgosmann
Copy link
Collaborator Author

jgosmann commented May 8, 2019

it would make the versioning less meaningful

That depends on what meaning one assigns to the version number. Is it about the amount of changes (by some measure, which seems the meaning Nengo assigns to some degree) or whether changes are breaking, adding features, or fixing bugs (the meaning that Semantic Versioning assigns). The former one might be more useful from a user perspective, the latter when trying to define a sensible version range for a dependency.

pytest frequently removes deprecated features even on patches

On a quick skim I could not find any removals on patch increments and the removals of deprecated features seem to be features that have been deprecated in the previous major version (and maybe were simply forgotten to be removed in the x.0.0 release).

But I guess it is not helpful to spend much more time on this discussion, but rather figure out what that means for this PR. Even though you are not strictly following Semantic Versioning, I suppose that a major version increment should be considered breaking. Thus, would it make sense to define the required Nengo version by Nengo GUI as >=2.8,<3? Or would you prefer >=2.8,<3.1 (and can you ensure that this would not break)?

This would be a quick and easy fix that ensures that a user gets a working install and IMO something like this should be done and released as a a first measure.

Then, should Nengo GUI be made compatible to Nengo 3 and if so by dropping Python 2.7 (and thus Nengo <3 support) or by moving the escape import code to Nengo GUI itself (thus supporting Nengo >2.8 including 3)?

Aside from the GUI, the same questions would apply to Nengo SPA.

@arvoelke
Copy link
Contributor

arvoelke commented May 9, 2019

The former one might be more useful from a user perspective, the latter when trying to define a sensible version range for a dependency.

I'm advocating for a reasonable balance between the two.

Thus, would it make sense to define the required Nengo version by Nengo GUI as >=2.8,<3?

That is what I did here: arvoelke/nengolib@1f3832c, and then my plan was to have nengolib==1.0.0 support nengo>=3.0.

@jgosmann
Copy link
Collaborator Author

Added a commit to set the upper Nengo version too.

@arvoelke arvoelke changed the title Set minimum Nengo version. Set minimum and maximum Nengo versions. May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants