Deprecate `sort` for `NTuple` and other iterables #811

martinholters · 2023-11-27T08:49:36Z

As this functionality was removed from Julia in JuliaLang/julia#52010.

CC @ericphanson

martinholters · 2023-11-27T08:50:37Z

~~This is on top of #810 to see whether tests now pass on nightly.~~ EDIT: Rebased after merging #810.

codecov · 2023-11-27T08:51:28Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (db2077e) 92.83% compared to head (e900807) 93.23%.

Files	Patch %	Lines
src/deprecated.jl	96.15%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #811      +/-   ##
==========================================
+ Coverage   92.83%   93.23%   +0.39%     
==========================================
  Files           2        3       +1     
  Lines         335      340       +5     
==========================================
+ Hits          311      317       +6     
+ Misses         24       23       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

martinholters · 2023-11-27T08:54:40Z

src/deprecated.jl

+# https://github.com/JuliaLang/julia/pull/46104
+# reverted in https://github.com/JuliaLang/julia/pull/52010
+if VERSION < v"1.10.0-DEV.1404" ||
+    (VERSION >= v"1.10-rc2" && VERSION < v"1.11.0-") ||


This assumes that JuliaLang/julia#52010 will actually be in v1.10-rc2.

martinholters · 2023-11-27T08:58:11Z

src/deprecated.jl

+    function Base.sort(v; kws...)
+        Base.depwarn("sorting arbitrary iterables is deprecated", :sort)
+        if v isa AbstractString
+            throw(ArgumentError("sort(::AbstractString) is not supported"))
+        end
+        if v isa NTuple
+            return _sort(v; kws...)
+        end
+        if v isa Tuple
+            throw(ArgumentError("sort(::Tuple) is only supported for NTuples"))
+        end


I've changed the dispatch to only add this single method to Base.sort. If part of this functionality is re-added toBase in the future, it will likely be for more specific argument types and we then won't overwrite these new methods. Adding this method Base for any future Julia versions still makes me a bit uncomfortable.

martinholters · 2023-11-27T08:58:56Z

test/runtests.jl

@@ -696,8 +696,6 @@ end
 @testset "sort(iterable)" begin
    function tuple_sort_test(x)
      @test issorted(sort(x))
-      length(x) > 9 && return # length > 9 uses a vector fallback
-      @test 0 == @allocated sort(x)


These now allocate due to depwarn.

martinholters · 2023-11-27T09:18:46Z

The sort tests still pass (except for the @allocated ones), but of course are quite noise with many depwarns now. Should we remove them?

KristofferC · 2023-11-28T11:23:33Z

No, this has to be reverted. We cannot have this level of type piracy in Compat just because it existed on Julia master for a while.

I mean, Compat should not even overload things in Base. You are supposed to have to call Compat.sort if you want to opt in to the "future" behavior of sort.

martinholters · 2023-11-29T08:03:02Z

No, this has to be reverted. We cannot have this level of type piracy in Compat just because it existed on Julia master for a while.

Yeah, that is super unfortunate, but if we just revert this, we have to do a new major version if we take SemVer seriously (which I like to do). And new major versions of Compat are not welcome by everyone, ref. #767 (comment). A middle ground might be to keep this (deprecated) in Compat but only for Julia < v1.10. That would mean that for sort on general iterables, you'd need Julia < 1.10. Not sure that's much better, but at least, Compat doesn't pollute Base forever then.

I mean, Compat should not even overload things in Base. You are supposed to have to call Compat.sort if you want to opt in to the "future" behavior of sort.

Hm, we've traditionally added methods to functions in Base whenever possible, reasoning that we're only doing it for past Julia versions where we know it's safe. And it's convenient from a user perspective. But I agree that always requiring the Compat. prefix would be much safer. We might want to adopt that in the future. But it doesn't help us with the mess we're in here now.

martinholters · 2023-11-29T08:06:13Z

src/deprecated.jl

+if VERSION < v"1.10.0-DEV.1404" ||
+    (VERSION >= v"1.10-rc2" && VERSION < v"1.11.0-") ||
+    VERSION >= v"1.11.0-DEV.924"


Suggested change

if VERSION < v"1.10.0-DEV.1404" ||

(VERSION >= v"1.10-rc2" && VERSION < v"1.11.0-") ||

VERSION >= v"1.11.0-DEV.924"

if VERSION < v"1.10.0-DEV.1404"

To stop adding methods to Base.sort in future Julia versions while keeping sort working on general iterables on Julia < 1.10 with Compat (where people might already be using it).

Ref. #811 (comment)

KristofferC · 2023-11-29T08:48:16Z

Can we at least run PkgEval on this change and see if it breaks something in practice and if not we just remove it? Having this deprecated (but still there) in all of < 1.10 is awful. It shouldn't even be there in the first place since this is a feature that was introduced in 1.10 and should not automatically be available on pre-1.10. We don't backport features for a reason.

The type of development where things that are tested on Julia master quickly get put into Compat in a type piracy way and then "cannot be removed due to semver" is completely unworkable. This package is depended on by a lot of other packages and any type piracy in it is very bad.

FWIW, I also think this change broke a package (https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/46617e5_vs_0ba6ec2/ITensorVisualizationBase.primary.log) where, ironically, their own type piracy (https://github.com/ITensor/NDTensors.jl/blob/19c079c554b546c57b3f5bd72db15c39112ff5d0/src/tupletools.jl#L162) stopped working.

So at least we have some evidence that the added type piracy here broke a package so based on semver (which we take seriously) it should be reverted.

ericphanson · 2023-11-29T11:03:37Z

In the past, “not matching Julia” has been called a bugfix btw: #745

martinholters · 2023-11-29T12:53:21Z

Yeah, these are good arguments for reverting.

Can we at least run PkgEval on this change

Can nanosoldier to this for us or would someone need to run it on their own box?

Meanwhile, searching for Compat = "4.9" only found NDTensors, so that's a likely candidate for being broken -- again.

martinholters commented Nov 27, 2023

View reviewed changes

martinholters mentioned this pull request Nov 27, 2023

sort for NTuple and other iterables #804

Merged

Deprecate sort for NTuple and other iterables

e900807

martinholters force-pushed the mh/deprecate-sort branch from f200489 to e900807 Compare November 28, 2023 07:06

martinholters marked this pull request as ready for review November 28, 2023 07:06

martinholters commented Nov 29, 2023

View reviewed changes

This was referenced Nov 29, 2023

Revert "sort for NTuple and other iterables (#804)" #812

Merged

Remove "sort for NTuple and other iterables" from README #813

Closed

Avoid type piracy by defining distinct functions #814

Open

martinholters closed this in #812 Dec 1, 2023

giordano deleted the mh/deprecate-sort branch January 20, 2024 10:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate `sort` for `NTuple` and other iterables #811

Deprecate `sort` for `NTuple` and other iterables #811

martinholters commented Nov 27, 2023

martinholters commented Nov 27, 2023 •

edited

Loading

codecov bot commented Nov 27, 2023 •

edited

Loading

martinholters Nov 27, 2023

martinholters Nov 27, 2023

martinholters Nov 27, 2023 •

edited

Loading

martinholters commented Nov 27, 2023

KristofferC commented Nov 28, 2023 •

edited

Loading

martinholters commented Nov 29, 2023

martinholters Nov 29, 2023

KristofferC commented Nov 29, 2023 •

edited

Loading

ericphanson commented Nov 29, 2023

martinholters commented Nov 29, 2023

Deprecate sort for NTuple and other iterables #811

Deprecate sort for NTuple and other iterables #811

Conversation

martinholters commented Nov 27, 2023

martinholters commented Nov 27, 2023 • edited Loading

codecov bot commented Nov 27, 2023 • edited Loading

Codecov Report

martinholters Nov 27, 2023

Choose a reason for hiding this comment

martinholters Nov 27, 2023

Choose a reason for hiding this comment

martinholters Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

martinholters commented Nov 27, 2023

KristofferC commented Nov 28, 2023 • edited Loading

martinholters commented Nov 29, 2023

martinholters Nov 29, 2023

Choose a reason for hiding this comment

KristofferC commented Nov 29, 2023 • edited Loading

ericphanson commented Nov 29, 2023

martinholters commented Nov 29, 2023

Deprecate `sort` for `NTuple` and other iterables #811

Deprecate `sort` for `NTuple` and other iterables #811

martinholters commented Nov 27, 2023 •

edited

Loading

codecov bot commented Nov 27, 2023 •

edited

Loading

martinholters Nov 27, 2023 •

edited

Loading

KristofferC commented Nov 28, 2023 •

edited

Loading

KristofferC commented Nov 29, 2023 •

edited

Loading