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

Deal with #2549 being merged in v1.13 but not v2.0 #2681

Closed
lbdreyer opened this issue Jul 12, 2017 · 11 comments
Closed

Deal with #2549 being merged in v1.13 but not v2.0 #2681

lbdreyer opened this issue Jul 12, 2017 · 11 comments

Comments

@lbdreyer
Copy link
Member

lbdreyer commented Jul 12, 2017

Originally posted by @bjlittle

@lbdreyer
Copy link
Member Author

lbdreyer commented Jul 12, 2017

I'm not convinced we can release v2.0 without #2459 [[#2549 - @pp-mo]].

share_data is a property of a public object (iris.cube.Cube) and therefore surely needs to be deprecated - it can't just vanish.

In our change management white paper, there is a line that states:

At a major release, only deprecated behaviours and APIs can be changed or removed.

@lbdreyer
Copy link
Member Author

We are actually breaking multiple parts of that change management white paper in this Iris v2.0 major release

@pp-mo
Copy link
Member

pp-mo commented Jul 13, 2017

@lbdreyer share_data is a property of a public object (iris.cube.Cube) and therefore surely needs to be deprecated - it can't just vanish.

I think, in retrospect, many of us felt that the APi introduced in #2549 was frankly unwise.
Owing to time pressure, it simply did not get the discussion it should have had.
I haven't looked deeply into the implications, but it may simply not be practical to deliver a backwards-compatible re-implementation.

The question of whether we are duty bound to honour #2549 is still somewhat open IMHO.
Because...

breaking multiple parts of that change management white paper

I think we are already obliged to re-write that paper, as dask simply breaks those rules, and shows why that is sometimes necessary -- or at least "preferable".
Also, as we discussed offline yesterday, there seem to be outright mistakes in that text anyway
(notably: deprecated behaviours should be removed in the following major release, not the one after that)
IMHO that "change whitepaper" was another thing that did not get enough scrutiny at the time.
(Excusing myself!)

Solutioneering ... At least one viable position is to revert to the SemVer view on major releases:

  • the only time that backwards-incompatible API changes occur
  • anything at all may change

I tried hard in #1999 to provide more assurance of continuity than that, and we probably should, but maybe only a declaration of intent, not firm rules.

@DPeterK
Copy link
Member

DPeterK commented Jul 14, 2017

I think we are already obliged to re-write that paper, as dask simply breaks those rules, and shows why that is sometimes necessary -- or at least "preferable".

I'm with @pp-mo on this. If we don't reduce the requirements of that whitepaper we will shackle ourselves into never being able to make major changes to Iris...

@marqh
Copy link
Member

marqh commented Jul 14, 2017

@lbdreyer @pp-mo
i find myself slightly confused by this issue (apologies if I am being slow)

i do not feel that
#2459 Remove skip_biggus from unit/fileformats/test_rules.py
or
#2489 #2489
are the change that is under discussion here. Please may you clarify that you are talking about a change which was added to master, and then v1.13.0, but is not present on the current master? Please may you clarify which pull request adopted this change?

thank you
mark

@marqh
Copy link
Member

marqh commented Jul 14, 2017

I think we are already obliged to re-write that paper, as dask simply breaks those rules, and shows why that is sometimes necessary -- or at least "preferable".

I'm with @pp-mo on this. If we don't reduce the requirements of that whitepaper we will shackle ourselves into never being able to make major changes to Iris...

@dkillick @pp-mo

This sounds like a very interesting conversation, which could well be a crucial activity for the iris2 milestone
Please may I suggest that a dedicated ticket on this topic is created, so valuable thoughts are shared in a well defined space.
We don't want that conversation to only occur half way down this issue

@ajdawson
Copy link
Member

ajdawson commented Jul 14, 2017

@marqh + others, the number in the title is correct, it is #2549. I think...

@marqh marqh mentioned this issue Jul 19, 2017
@marqh
Copy link
Member

marqh commented Jul 19, 2017

thanks @ajdawson that makes sense

I think this behaviour should be preserved, it has been adopted into the core Iris API at v1.13

The implementation is not complicated, see #2691 for details

@DPeterK
Copy link
Member

DPeterK commented Jul 20, 2017

I think this behaviour should be preserved

Maybe the behaviour should be preserved, but the proposed implementation needs serious work before I'll be happy to accept it into Iris v2.

@DPeterK
Copy link
Member

DPeterK commented Jul 20, 2017

I think we are already obliged to re-write that paper

a dedicated ticket on this topic is created

Regarding updating Iris' change management principles, see #2692.

@pelson
Copy link
Member

pelson commented Nov 2, 2017

Resolved by what's new item for 2.0.0. In particular:

* Due to concerns regarding maintainability and API consistency the
  :attr:`iris.cube.Cube.share_data` flag introduced in v1.13 has been removed.
  Intra-cube data sharing is a oft-requested feature that we will be targeting
  in a future Iris release.

It isn't ideal, but a pragmatic choice to allow us to move forwards with iris 2 in a long-term sustainable way.

The feature PR itself is still open in #2691.

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

No branches or pull requests

6 participants