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

Add skip-4 draft #6339

Merged
merged 14 commits into from Apr 22, 2022
Merged

Add skip-4 draft #6339

merged 14 commits into from Apr 22, 2022

Conversation

jni
Copy link
Member

@jni jni commented Apr 12, 2022

Description

This is the draft for SKIP-4, the alternative to SKIP-3 (#5475). It proposes the release of a new API under the skimage2 namespace and package name.

Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>
Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

Thanks, Juan. This seems in line with what we discussed in prior community meetings.

One other aspect we did discuss previously was potentially having a skimage 1.1 that is identical to 1.0, but adds a warning on import that points users to the new skimage2 package. Users wanting to remain on scikit-image 1.0 could then just pin their requirements to skimage<1.1 to avoid this warning.


- scikit-image 0.19 will be followed by scikit-image 1.0. Every deprecation
message will be removed from 1.0, and the API will be considered the
scikit-image 1.0 API.
Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify by "every deprecation message will be removed", you mean that just the warnings will be removed, but the actual deprecations will not be completed?. For instance, rather than removing multichannel in favor of channel_axis, both would be left in the 1.0 API? (to avoid having a shorter than usual deprecation cycle for deprecations that were new to v0.19).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it's ambiguous, and I agree with your assessment: let's make only the deprecations slated for 0.20, and remove other warning messages, even if it means a duplicate API (as in the case of multichannel vs channel_axis).

scikit-image 1.0 API.
- After 1.0, the main branch will be changed to (a) change the import name to
skimage2, (b) change the package name to skimage2, and (c) change the version
number to 2.0-dev.
Copy link
Contributor

Choose a reason for hiding this comment

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

We could technically make the skimage2 change on main immediately as we already merged some API changes there which aren't scheduled for 1.0.

We could branch v1.0.x from v0.19.x branch which has had no API changes, but does have bacports of all bug fixes since 0.19.2. We should likely make a v0.19.3 with the current bug fixes in that branch and then prepare a 1.0 very soon afterward that removes the deprecation messages.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I would like to include the present changes, as long as there are no new deprecation messages, in 1.0. ie I don't think we need or want to branch off 0.19.

doc/source/skips/4-transition-to-v2.rst Outdated Show resolved Hide resolved
Co-authored-by: Gregory Lee <grlee77@gmail.com>
@hmaarrfk
Copy link
Member

This seems really positive!

Copy link
Member

@lagru lagru left a comment

Choose a reason for hiding this comment

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

Thanks @jni for taking this on!

doc/source/skips/4-transition-to-v2.rst Outdated Show resolved Hide resolved
doc/source/skips/4-transition-to-v2.rst Show resolved Hide resolved
doc/source/skips/4-transition-to-v2.rst Outdated Show resolved Hide resolved
@lagru lagru added this to the skimage2 milestone Apr 13, 2022
jni and others added 3 commits April 13, 2022 20:26
Co-authored-by: Lars Grüter <lagru+github@mailbox.org>
Co-authored-by: Lars Grüter <lagru+github@mailbox.org>
maintainer time, or a long deprecation cycle for the 1.0 namespace, which would
ultimately result in a lot of unhappy users getting deprecation messages from
their scikit-image use.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do like separate packages better, but here are two concrete examples that did go with this combined package approach:

  • OpenCV had both cv and cv2 namespaces available in the same package for a little while.
  • imageio recently introduced imagio.v2 and imageio.v3 with the plan that the default imageio namespace will switch from v2 to v3 at a future date.

Copy link
Member

Choose a reason for hiding this comment

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

BeautifulSoup is also a good example, I guess.

skimage2, (b) change the package name to skimage2, and (c) change the version
number to 2.0-dev.
- There will be *no* scikit-image package on PyPI with version 2.0. Users who
``pip install scikit-image`` will always get the 1.0 version of the package.
Copy link
Member

Choose a reason for hiding this comment

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

IIRC that's one point we discussed before; to install skimage2 people would use pip install skimage2, is that 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, that's what "changing the package name" (vs import name) means. Will try to clarify.

@alexdesiqueira alexdesiqueira marked this pull request as draft April 14, 2022 20:40
My initial goal was to include all points and related context in this
draft. So right now there may be some tangential or not strictly
necessary information here. In any case I felt the structure and content
is a good starting point in case somebody wants to add something.
@lagru
Copy link
Member

lagru commented Apr 17, 2022

Citing the commit: My initial goal was to include all points and related context in this draft. So right now there may be some tangential or not strictly necessary information here. In any case I felt the structure and content is a good starting point to improve upon.

@jni
Copy link
Member Author

jni commented Apr 17, 2022

Thanks @lagru! I'm quite happy to err on the side of more information rather than too little information. In the future, I'm sure it will be nice for someone to be able to quickly find what was considered.

@jni jni marked this pull request as ready for review April 17, 2022 13:00
@jni
Copy link
Member Author

jni commented Apr 17, 2022

@scikit-image/core imho this is ready to merge. Note this line from SKIP-0:

A SKIP that outlines a coherent argument and that is considered reasonably complete should be merged optimistically, regardless of whether it is accepted during discussion.

We can then discuss accepting the SKIP on the discussion forum.

scikit-image2 might be an evolution of the package name but not of the import name!
lagru and others added 4 commits April 18, 2022 17:45
This plan was discussed earlier and brought up again by Gregory Lee.
This is already covered more extensively in the section "Related Work".
@jni
Copy link
Member Author

jni commented Apr 19, 2022

Ok the build is passing, last two failures are segfaults apparently caused by simpleitk, unrelated to this PR.

@jni
Copy link
Member Author

jni commented Apr 22, 2022

Anyone want to approve/merge this? 😊

Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks, @jni. Let's go ahead and merge. Do you want to make a couple of emails and/or forum posts to raise awareness and get feedback from the community?

@grlee77 grlee77 added the 📄 type: Documentation Updates, fixes and additions to documentation label Apr 22, 2022
@grlee77 grlee77 merged commit d52da74 into scikit-image:main Apr 22, 2022
@jni jni deleted the skip4 branch April 22, 2022 13:52
@jni
Copy link
Member Author

jni commented Apr 22, 2022

Yup, that's the next step! Whoo! 😊

@alexdesiqueira
Copy link
Member

Thank you @jni!

@stefanv
Copy link
Member

stefanv commented Apr 25, 2022

Thanks for writing this up, @jni! I think you did a great job. I don't know where else to leave comments, so here they are:

  • The introductory paragraph of the abstract could be much clearer; right now, it does not talk very much about the SKIP.
  • Releasing 1.0 and 1.1 and maintaining both seems like an unnecessary burden (and a big one!). People who want to switch off the warning can do so with a filter, and we can provide a utility function too. We can even respect an environment flag.
  • You mention that "That said, the authors will attempt to limit the number of backward incompatible changes to those likely to substantially improve the overall user experience." I think we should make a stronger statement, and also mention this earlier in the document: We will explicitly justify each and every API change, and will generally prefer deprecation when it is a viable route.
  • Can we include more links to the previous discussions around SKIP-3 and the skimage2 proposal? (We had a long mailing list thread, e.g.)

Thanks again!

@jni
Copy link
Member Author

jni commented May 2, 2022

Hey @stefanv!

The introductory paragraph of the abstract could be much clearer; right now, it does not talk very much about the SKIP.

Could you tell me a bit more about what you would like to see here? Do you want the first paragraph to summarise the whole skip, and subsequent paragraphs to explain motivation?

Releasing 1.0 and 1.1 and maintaining both seems like an unnecessary burden (and a big one!).

I don't think it's a big one: rather than maintaining 1.1.x and 1.0.x branches, we only maintain a 1.0.x. Every 1.0.x release gets a single commit cherry-picked on top of it that adds the warning, and released as 1.1.x.

I find manual silencing of warnings is quite burdensome to be honest, and would like to avoid that for our users.

We will explicitly justify each and every API change, and will generally prefer deprecation when it is a viable route.

I disagree with this statement though: there will be no deprecations between 1.x and 2.0, only API changes. But the first part, I can definitely get behind: every change will be explicitly justified.

Can we include more links to the previous discussions around SKIP-3 and the skimage2 proposal? (We had a long mailing list thread, e.g.)

I vaguely preferred not duplicating the discussion section from SKIP-3, but I can do so if you feel strongly about it.

@stefanv
Copy link
Member

stefanv commented May 2, 2022

I think "links to" won't duplicate.

W.r.t. maintaining two versions, I suppose it's okay if it only requires one patch.

W.r.t. the first paragraph, exactly: big picture first, then detail.

🙏🏻

@stefanv
Copy link
Member

stefanv commented Jan 27, 2023

Should I add the migration plan from https://discuss.scientific-python.org/t/a-pragmatic-pathway-towards-skimage2/530/11 here, or make a new SKIP, @jni?

@lagru
Copy link
Member

lagru commented Jan 30, 2023

My 2 cents, if in doubt it's probably less confusing if we create yet another SKIP 😅 and keep discussions around SKIP4 from being confusing. Nothing's stopping us from copying the still relevant parts from SKIP4. Thanks for following up on this @stefanv!

@jni
Copy link
Member Author

jni commented Jan 31, 2023

I'm actually more in favour of modifying this SKIP (SKIP-4), as it's still in draft, and I never made the changes requested by @stefanv above. In my opinion it's more confusing to have seven not-quite-right migration SKIPs than to have two: the failed one and the successful one.

@jarrodmillman jarrodmillman modified the milestones: skimage2, 0.20 Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 type: Documentation Updates, fixes and additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants