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

Userguide section and whitepaper on user change management. #1999

Merged
merged 3 commits into from
Jul 27, 2016

Conversation

pp-mo
Copy link
Member

@pp-mo pp-mo commented May 3, 2016

Suggestions for clarifying change management from the user POV
= releases + versions, deprecations, iris.FUTURE

This is mostly for discussion at this point.
It is now pretty much complete + correct to my original conception.
However, @rhattersley already suggested that this information might with advantage be a bit "generalised" and then "pushed back" onto the SemVer project https://github.com/mojombo/semver.
As I was most of the way toward providing this a standalone Iris docs, I've just carried on with that for this first draft.

This issue is mentioned as a possible for 1.10, #1948
which would be nice as we are targetting a lot of deprecations in 1.10.
While may be relevant there, it certainly isn't essential.

@@ -0,0 +1,9 @@
To reduce code maintenance problems to an absolute minimum, the Iris project defines
carefully defined change management procedures to ensure that :
Copy link
Member

Choose a reason for hiding this comment

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

defines carefully defined? A bit repetitive.

@pp-mo
Copy link
Member Author

pp-mo commented May 4, 2016

if I import iris and look for names without a single leading underscore ... iris sub-packages ...

It's surely ok to exclude local imports, though it certainly is an annoying glitch and a backwards incompatibility if code fails because we removed an automatic submodule import

E.G. what if importing iris did not result in importing iris.cube ?

I think @rhattersley already suggested that all internal imports could be given private internal names, to avoid this kind of problem.
Changing that now will doubtless upset lots of existing user code, but it we want it we should get on right now and introduce a deprecation for it in 1.10

I've put a note in the 1.10 ticket #1948 for now
However, I'm not sure if this is actually feasible as it stands ...

>>> import iris
>>> hasattr(iris, 'proxy')
False
>>> from iris import proxy as _proxy
/home/h05/itpp/git/iris/iris_main/lib/iris/proxy.py:34: UserWarning: iris.proxy is deprecated in Iris v1.9. Please use lazy imports instead.
  warnings.warn('iris.proxy is deprecated in Iris v1.9. Please use lazy '
>>> hasattr(iris, 'proxy')
True
>>> 

@rhattersley
Copy link
Member

I think @rhattersley already suggested that all internal imports could be given private internal names, to avoid this kind of problem.

I'm now 👎 on this. I mentioned the idea yesterday offline, but it doesn't address the whole problem. Instead I think it'd be better to use the existing __all__ convention.

@pp-mo
Copy link
Member Author

pp-mo commented May 4, 2016

Thanks @rhattersley
This is a jolly useful discussion (for me, anyway)

But they also state:

If __all__ is not defined, the set of public names includes all names found in the module’s namespace which do not begin with an underscore character ('_').

So we'd need to start being a bit more thorough with our __all__ definitions.

All true: "thorough" is strictly wanted, but for here+now all that really matters is what we include in _our_ definition of "public"

  • and I now think this is a good candidate to include.
    Given which, I think I should enhance the formal definition in the Developer's Guide, and just reference that here rather than repeating all the gory details : Applies also to the point you already made about experimental et al.

@pp-mo
Copy link
Member Author

pp-mo commented May 4, 2016

However, I'm not sure if this is actually feasible as it stands ...

> import iris
> hasattr(iris, 'proxy')
> from iris import proxy as _proxy

To be clear, this example really isn't quite the point...
Though you can't avoid (e.g.) iris having a 'proxy' property if it has ever been imported,
the real problem is more like this one :

>>> import iris.cube
>>> iris.cube.datetime.MIN_YEAR
1

I.E. 'datetime' appears as a public property of iris.cube.
That we can avoid.

@pp-mo
Copy link
Member Author

pp-mo commented May 4, 2016

Note: I'm going to let this stew a bit longer before doing any more, in case there are further comments.

@pp-mo
Copy link
Member Author

pp-mo commented May 4, 2016

@QuLogic thanks, not much to say : all your comments were true !

@pp-mo
Copy link
Member Author

pp-mo commented May 6, 2016

@rhattersley suggested I could consider pushing some of this content back into the SemVer document.
So I thought about what here is SemVer and what not.

My conclusions are...

SemVer provisions:

  • backwards-incompatible changes can only occur at major releases
  • additions and changes to the public API can only be introduced at a minor release (not bugfix)
  • deprecations to functionality can only be introduced at a minor release (not bugfix)
  • there are no definitions of certain terms used (which at least ensures language-independence) :
    • API
    • public
    • behavior / functionality
    • deprecation (what "marking API functionality as deprecated" exactly means)

Additional provisions from Iris (as I'm attempting to rationalise them):

  • key certainties we are adding:
    • only deprecated features may be changed at a major release
    • deprecated functionality will always emit runtime deprecation warnings
      • ( for user code that 'could' invoke deprecated functionality, e.g. maybe with different data )
    • it is always possible to avoid deprecations
    • it is always possible to avoid deprecation warnings
    • these then ensure ... :
      • code that ran in the version preceding a major release, without deprecation warnings, will continue to work after a major release
      • you can always write code that will work both before and after a release (even a major release)
  • our deprecations
    • will always be flagged in relevant documentation
    • will always raise runtime warnings for code where it 'could' make a difference
      • ( ? though it still isn't clear to me whether we can always deliver this ? )
  • use of iris.FUTURE : a recommended central control for compatibility-breaking modal changes
  • terms we are trying to define more precisely :
    • what is "public API"
    • "backwards compatible" : means any conceivable existing code should "behave" as previously
    • "behaviour" : means return values and side effects
      • ( ? but this probably needs to be limited a bit : "excluding warnings and printed output", or something like that ? )

major and minor version number in the version name, "major.minor.xxx",
as explained in the :ref:`releases section <iris_change_releases>` below.

* Code that currently works should **still work**, and behave exactly the
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 a fallacy. It is not possible to change anything without breaking something else. https://xkcd.com/1172/

@pp-mo
Copy link
Member Author

pp-mo commented May 11, 2016

This is a fallacy. It is not possible to change anything without breaking something else. https://xkcd.com/1172/

Classik ! 😀

But Seriously..
I had a good chat with @rhattersley yesterday about separating off iris.fileformats.grib into a separate package/project, as you suggested : so that we can release Iris 1.10 without needing to first resolve all the ongoing efforts to deprecate parts of it and provide alternatives for everything.

( The idea would be to spinoff "iris_grib", initially equivalent to the current "iris.fileformats.grib", but then evolving seperately.
In particular, we can then deprecate and remove the "old" loader without requiring an Iris major version release. )

This example makes clear a point which I had not properly considered in the proposals here :
Any "behaviour" (as here, i.e. all outcomes of any code) can potentially change when any dependency is changed (e.g. upgraded), to the point of breaking (e.g. dependency major version change).

That means we can only completely fossilize behaviour by pinning the version of all dependencies (that is, at least their major versions, where the concept applies).
Which seems pretty crazy and meaningless.
Instead, I guess the correct approach is to redefine "behaviour" in terms of the calls on component dependencies, not total results ??
Any ideas how best to state this ?

I think we should still accept that if a version upgrade in a dependency actually breaks something, then that is a bug and we need to limit that version until we fix it.
This is currently true for matplotlib and numpy : we aren't fully compatible with latest versions.

@pp-mo
Copy link
Member Author

pp-mo commented Jul 14, 2016

@marqh what are your thoughts on this ?
I'd like us to firm up on it, but this may be altogether too wordy

@pp-mo
Copy link
Member Author

pp-mo commented Jul 14, 2016

Latest revisions...

  • I have attempted to respond to all the simple improvement suggestions already posted
  • I have attempted to define more precisely what we mean by "public"
  • I have attempted to define more vaguely (!) what we mean by "same behaviour"
  • I haven't yet taken any position on the PEP440 / SemVer version string formatting issue.

@pp-mo
Copy link
Member Author

pp-mo commented Jul 14, 2016

Other issues mentioned in discussion above, but still not addressed in the text :

  • imported modules-within-modules (as excluded from "public" API)
  • segregated description of SemVer principles, and what we add to them

@marqh marqh added the ready label Jul 15, 2016
@marqh
Copy link
Member

marqh commented Jul 26, 2016

ping @marqh

@@ -0,0 +1,9 @@
To reduce code maintenance problems to an absolute minimum, Iris applies
Copy link
Member

Choose a reason for hiding this comment

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

i recommend removing these sections from the user guide

this section is already in use in the white paper, so the content is preserved.

@@ -0,0 +1,38 @@
Code Maintenance
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 this is covered in the white paper. This section could be removed, to try and keep the user guide more tightly focussed

@marqh
Copy link
Member

marqh commented Jul 26, 2016

i think it is worth considering keeping this to the white paper, and not adding a section to the user guide

how much benefit do we get from the extra user guide section as well?

@pp-mo
Copy link
Member Author

pp-mo commented Jul 26, 2016

not adding a section to the user guide

I agree the userguide is rather bloated now.
My concern is that this topic should definitely be on a list of "things the user should know, that they may not know they need to know".
Can you suggest a better approach that satisfies that need ?

The userguide contents listing was just the only existing place I could think of that performs that function. Obviously, though, it does include a good few chapters that are firmly in the "specialist interest : come back later" category.
Right now, we just don't have a simple 'read this first' section. Perhaps we should ? ...

@marqh
Copy link
Member

marqh commented Jul 27, 2016

ok, I'm sold; let's accept this now. Any reworking of the docs can include a review of

Right now, we don't have a clear 'read this first' section at present, and perhaps we should...

@marqh marqh merged commit fd53eb6 into SciTools:master Jul 27, 2016
@marqh marqh removed the ready label Jul 27, 2016
@QuLogic QuLogic added this to the v1.10 milestone Jul 27, 2016
@pp-mo pp-mo deleted the change_management_guide branch March 18, 2022 15:35
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