WIP: move csparse functions to separate package #16070

KristofferC · 2016-04-27T11:00:45Z

This PR lifts out the LGPL licensed csparse functions into https://github.com/JuliaSparse/CSparse.jl which is a slightly updated version of @dmbates package https://github.com/dmbates/CSparse.jl.

TODO / Issues (updated):

Register SuiteSparse
~~Register CSparse~~
~~CSparse currently says that it has a MIT license. Does this need to be changed to LGPL?~~
~~Tests on 0.5 fail because some of the prerequisites of Metis fail on 0.5 with their last tagged version. Is it a problem that the functions are moved to a package with a binary dependency?~~
Since we still need to export etree and symperm to throw the error message, it is difficult to override these functions from CSparse. Currently I just redefine the function but this is not so clean because it generated a warning. Maybe better to not export the functions from CSparse and tell people to use CSparse.etree for example?
~~I modified the etree function in CSparse to be the same as the one in base. Is that ok with you @dmbates~~

cc @tkelman @Sacha0

[edit by tkelman: closes #12231]

tkelman · 2016-04-27T11:11:16Z

.0. Should stay WIP until CSparse gets registered. (github lists don't like being 0-based)

If there's copied/ported LGPL code, then yes at least the files that contain that code should be LGPL and the top-level license should mention as much. Maybe better to relicense the entire package to LGPL for simplicity if all contributors agree.
That could be a bit of a problem. Maybe have CSparse be the simpler all-Julia package (and/or ccalling into Julia's copy of SuiteSparse where necessary for now, subject to change again in the future when SuiteSparse moves to its own package). Then the Metis package could depend on CSparse relatively harmless-ly.
I like just qualifying usage as CSparse.etree for a while. I don't think people who know how to find and really make full use of these are deeply worried about keystrokes on this.

.5. actually come to think of it should we just call the package SuiteSparse.jl in preparation for putting all the GPL binary dependencies there later?

KristofferC · 2016-04-27T11:21:07Z

How about creating a new SuiteSparse package that for now only contains the moved functions? This would remove the dependency problem and the fact that some of the functions in CSparse is currently untested + undocumented. Like you say, this package would then later be used to put the rest of the binary SuiteSparse stuff in.

KristofferC · 2016-04-27T11:26:02Z

OK. I'm kinda lost when it comes to licenses. Should I just have GPL for the SuiteSparse package then? Is LGPL and GPL compatible so I can put the CSparse stuff in there (CSparse is LGPL)?

Found this so should be ok

Note that the LGPL is compatible with the GPL: you can opt to "upgrade" the code to GPL and incorporate it in a wholly GPL licensed project as set out in my first bullet point if you wish. You can't however go the other way and re-license GPL licensed code as LGPL.

tkelman · 2016-04-27T11:51:33Z

The Julia code should be LGPL as that's less restrictive. Once we start putting binary dependencies there that include the GPL pieces of SuiteSparse we will mention as much in the package's license (that it will download/build GPL binaries or source code on installation) but the Julia code there can stay LGPL. Or if we really want to get particular we can segregate LGPL-derived Julia code from even more permissive from-scratch MIT code, but I think that would be overkill.

KristofferC · 2016-04-27T12:12:30Z

Please see https://github.com/JuliaSparse/SuiteSparse.jl

KristofferC · 2016-04-27T12:13:08Z

doc/stdlib/arrays.rst

@@ -971,12 +971,16 @@ dense counterparts. The following functions are specific to sparse arrays.

   Compute the elimination tree of a symmetric sparse matrix ``A`` from ``triu(A)`` and, optionally, its post-ordering permutation.

+   Note: This function has been moved to the CSparse.jl package.


Will update this

tkelman · 2016-04-27T12:33:14Z

base/sparse.jl

+Note: This function has been moved to the SuiteSparse.jl package.
+"""
+symperm{Tv,Ti}(A::SparseMatrixCSC{Tv,Ti}, pinv::Vector{Ti}) =
+    error("symperm(A, pinv)", SuiteSparse_END_ERR_SRING)


SUITESPARSE_END_ERR_S[T]RING

yeah... speaking of that, should these errors be tested?

maybe, I can't remember whether the PkgDev or similar recent migrations had tests for the migration errors

looks like they did

julia/test/pkg.jl

Lines 208 to 256 in c8b9fe9

#test PkgDev redirects

begin

try

Pkg.register("IDoNotExist")

error("unexpected")

catch ex

@test ex.msg == "Pkg.register(pkg,[url]) has been moved to the package PkgDev.jl.\nRun Pkg.add(\"PkgDev\") to install PkgDev on Julia v0.5-"

end

try

Pkg.tag("IDoNotExist")

error("unexpected")

catch ex

@test ex.msg == "Pkg.tag(pkg, [ver, [commit]]) has been moved to the package PkgDev.jl.\nRun Pkg.add(\"PkgDev\") to install PkgDev on Julia v0.5-"

end

try

Pkg.generate("IDoNotExist","MIT")

error("unexpected")

catch ex

@test ex.msg == "Pkg.generate(pkg, license) has been moved to the package PkgDev.jl.\nRun Pkg.add(\"PkgDev\") to install PkgDev on Julia v0.5-"

end

try

Pkg.publish()

error("unexpected")

catch ex

@test ex.msg == "Pkg.publish() has been moved to the package PkgDev.jl.\nRun Pkg.add(\"PkgDev\") to install PkgDev on Julia v0.5-"

end

try

Pkg.license()

error("unexpected")

catch ex

@test ex.msg == "Pkg.license([lic]) has been moved to the package PkgDev.jl.\nRun Pkg.add(\"PkgDev\") to install PkgDev on Julia v0.5-"

end

try

Pkg.submit("IDoNotExist")

error("unexpected")

catch ex

@test ex.msg == "Pkg.submit(pkg[, commit]) has been moved to the package PkgDev.jl.\nRun Pkg.add(\"PkgDev\") to install PkgDev on Julia v0.5-"

end

try

Pkg.submit("IDoNotExist", "nonexistentcommit")

error("unexpected")

catch ex

@test ex.msg == "Pkg.submit(pkg[, commit]) has been moved to the package PkgDev.jl.\nRun Pkg.add(\"PkgDev\") to install PkgDev on Julia v0.5-"

end

end

but this is a bit more obscure, so up to you

The errors and docstrings look fine locally and these methods are probably not used at all as much as the Pkg stuff so this might not need to be tested.

StefanKarpinski · 2016-04-27T13:49:34Z

Note that the Julia code wrapping a L/GPL package can be MIT-licensed as long as the code itself is not derived from L/GPL code. In other words, if you port an L/GPL C library to Julia, then the Julia version must be L/GPL as well. If you wrap an L/GPL C library in Julia, on the other hand, then the wrapper code can be MIT. In both cases, the combined product will be L/GPL but in the latter case, someone could derive something from the wrapper code without the L/GPL library and the combination could be MIT.

tkelman · 2016-04-27T14:03:52Z

This particular Julia code was ported directly from the underlying C code, AIUI. The ccall wrappers that we will likely move into this package later are MIT though, so the SuiteSparse.jl package should probably keep track of which content comes from where as things get moved and added to it.

KristofferC · 2016-04-27T14:07:28Z

So what is the license of the whole package then? Or does the package itself not need a license but only the individual parts of the package?

tkelman · 2016-04-27T14:13:34Z

(IANAL) The whole package would be LGPL as that's the most restrictive of any licenses that the Julia code in the package will have. Any MIT licensed code that gets moved there has to be acknowledged and the MIT pieces listed in the license file as well, but MIT and LGPL are compatible so that's fine. Any redistribution of the package that also includes GPL-licensed binaries (the Julia code on github will not fall into this category, but if you download the package to your machine then re-upload the installed version including binaries post-Pkg.build then you would) would need to be GPL licensed.

edit: there's a diagram at https://en.wikipedia.org/wiki/License_compatibility#Kinds_of_combined_works - makes you wanna go back to lab and solve A x = b a few more times, doesn't it?

StefanKarpinski · 2016-04-27T14:29:38Z

To understand licensing better, you have to change perspective a little. A license isn't actually a property of code. When we say "this code has X license" it's really shorthand for "permission has been granted by the copyright holders of this code to use it if you abide by the terms of the X license". Licenses apply to your actions, not the code. If your actions meet the requirements of a license then you can use the code. In this case, the MIT license requires you to do some things to use some parts of the code and L/GPL requires you to do all of that and more to use other parts of the code. Thus, as long as you follow the requirements of the L/GPL, you're also satisfying the requirements of the MIT license, so you can use everything. The shorthand for that situation is "this combined code is L/GPL" – but really the parts that were licensed MIT before are still licensed MIT.

License conflicts arise when there is no way for your actions to satisfy the terms of both licenses. This would be the case if we shipped Julia with GPL libraries and proprietary ones like MKL. The code can exist, but you can't legally use it since no matter what you do, you're violating one of the licenses, making it illegal for you to use some part of the system.

There can also be situations where two licenses don't conflict but neither is a strict subset of the other. In that case you must abide by all the requirements of both licenses in order to be allowed to use the code.

Sacha0 · 2016-04-27T17:52:00Z

The diff looks good to me, but my review experience is practically nil. Best!

hayd · 2016-04-27T18:09:44Z

Should these be deprecated rather than raise an error...

Since we're going to be in the business of moving more things out of Base (#5155) perhaps it would it will make more sense to deprecate with something like (a "soft dependency deprecation"):

Try to import CSparse: symperm and call it, otherwise raise the error (note that the function should be deprecated in both cases).

tkelman · 2016-04-28T09:04:36Z

I think it would have to be a runtime import, which sounds like it could be pretty slow.

tkelman · 2016-04-30T09:35:18Z

Is the package in a state that it can be registered yet? Would be good to at least move over the rendered docs into the readme if that's not too much work.

KristofferC · 2016-04-30T09:46:43Z

Yes, I will do that in one of the upcoming days.

KristofferC · 2016-05-02T11:35:09Z

Rendered documentation added to the repo.

KristofferC · 2016-05-02T11:35:27Z

Still don't know how this is supposed to be deprecated cleanly.

tkelman · 2016-05-03T05:16:50Z

Something like

if isdefined(Main, :SuiteSparse)
    SuiteSparse.f(...)
else
    error("f(...)", SUITESPARSE_END_STRING)
end

?

hayd · 2016-05-03T06:07:11Z

you could try to import instead (see #12697)

tkelman · 2016-05-03T06:12:37Z

I suspect import and a try-catch would be much slower than isdefined.

edit: this is also in Base, where we shouldn't be importing things that may or may not exist at build time and compiling the result of that into the system image.

KristofferC · 2016-05-03T06:13:37Z

But then it will error for people not having the package? The point of a depreciation is that the code should still work, just give a warning, no?

tkelman · 2016-05-03T06:15:45Z

Usually, yes. But I don't think it's possible to do that here without completely duplicating the code for a release cycle. Past cases where we've moved code (DArrays, PkgDev, some of Combinatorics, etc) has pretty much always been by throwing errors pointing to the new location, I believe.

The functions and tests for these function have been moved to the package SuiteSparse.jl in the JuliaSparse organization.

KristofferC · 2016-05-14T09:12:43Z

I have updated this with the deprecation strategy @tkelman suggested (with a minor modification to make it work).

KristofferC · 2016-05-14T09:13:25Z

One question is what to do about the docstrings?

tkelman · 2016-05-14T10:38:26Z

Copy them over into the readme of the new package?

KristofferC · 2016-05-14T10:42:15Z

Yes I have done that. I'm talking about here.

tkelman · 2016-05-14T13:38:13Z

I think we can remove documentation when we deprecate something? The deprecations should probably also be in deprecated.jl otherwise we'll forget to delete them for a future release.

KristofferC · 2016-05-14T13:56:39Z

I put them there because the PkgDev stuff was in pkg.jl but I can move them.

tkelman · 2016-05-14T14:07:19Z

actually I'm not so sure now, since that would move them to a different module, and at least the unexported ones may have been called fully qualified. though maybe that's fair game to break. At least take the module level exports for these and put them next to the deprecated definitions?

tkelman · 2016-05-24T04:07:48Z

What's the current thinking here? Either we remove them completely and just leave error-throwing (or attempt-to-import?) stubs that point to the package, or maybe take a strategy similar to some of the intermediate commits in #16481 where we leave the current implementations present in base but move them to a deprecated-csparse.jl where they continue working but throw a deprecation warning pointing to the package.

tkelman · 2016-06-04T04:00:50Z

Bump.

ViralBShah · 2016-06-04T04:01:39Z

Are we going to make this happen for 0.5? Should be possible and we should tag it 0.5 then.

tkelman · 2016-06-04T04:05:06Z

It's not release blocking but it's nice to have. You weren't on towards the end of the triage call, but we should really stop adding new issues to the milestone if we want to make it to a feature freeze any time soon.

KristofferC · 2016-06-04T07:39:21Z

Im at a conference for a week so won't have any time to work on this until I am back.

Sacha0 · 2016-06-14T19:54:05Z

What's the current thinking here? Either we remove them completely and just leave error-throwing (or attempt-to-import?) stubs that point to the package, or maybe take a strategy similar to some of the intermediate commits in #16481 where we leave the current implementations present in base but move them to a deprecated-csparse.jl where they continue working but throw a deprecation warning pointing to the package.

After #16931, only symperm, etree, and ereach are left if I'm not mistaken. symperm I hope to tackle in the next week or two. etree and ereach are sufficiently esoteric that any effort beyond deprecating with an error-throwing stub strikes me as misspent. Hence I would advocate for merger essentially as-is, or even without the isdefined snippets alongside the error-throwing stubs --- whatever is easiest for @KristofferC.

Anything I can do to lighten the load here, @KristofferC? Best!

KristofferC · 2016-06-14T20:08:16Z

I'm not really sure what should be done here but when someone decides you could just take over this branch and open your own PR with the adjustments needed :)

hayd · 2016-06-14T22:32:01Z

So since SuiteSparse is published on METADATA, does this mean we just need to decide what the correct way to deprecate/remove is?

tkelman · 2016-06-14T22:34:33Z

Yep, and someone to take the time to adopt and finish this, whatever the decision is. My personal least-preferred option is attempting to import anything from a package into base, I don't think we've had any well-executed examples of that.

hayd · 2016-06-14T22:42:11Z

Are there any complaints to merging as-is (post rebase)?

Edit: Sorry, I'd not seen #16931, looks like that supersedes this? Or at least should be considered first!

tkelman · 2016-06-14T22:50:27Z

Better to delete the docs and move the error-throwing stubs to deprecated.jl I think. Maybe not even bother with the isdefined checks.

tkelman · 2016-06-20T18:42:01Z

replaced by #17033

KristofferC force-pushed the kc/del_csparse branch from 2a05ec2 to 3fa0e3a Compare April 27, 2016 11:55

KristofferC reviewed Apr 27, 2016
View reviewed changes

KristofferC force-pushed the kc/del_csparse branch from 3fa0e3a to 6c83a1d Compare April 27, 2016 12:17

tkelman reviewed Apr 27, 2016
View reviewed changes

KristofferC force-pushed the kc/del_csparse branch from 6c83a1d to 3417a1e Compare April 27, 2016 12:35

ViralBShah added the sparse Sparse arrays label May 2, 2016

remove csparse functions

d415859

The functions and tests for these function have been moved to the package SuiteSparse.jl in the JuliaSparse organization.

KristofferC force-pushed the kc/del_csparse branch from 3417a1e to d415859 Compare May 14, 2016 09:12

KristofferC mentioned this pull request May 14, 2016

Register SuiteSparse JuliaLang/METADATA.jl#5172

Merged

Sacha0 mentioned this pull request Jun 14, 2016

MIT-licensed SparseMatrixCSC permute[!] and refactored [c]transpose[!] #16931

Merged

Sacha0 mentioned this pull request Jun 20, 2016

Move CSparse functions to separate package, take two #17033

Merged

tkelman closed this Jun 20, 2016

KristofferC deleted the kc/del_csparse branch June 21, 2016 03:11

		@@ -971,12 +971,16 @@ dense counterparts. The following functions are specific to sparse arrays.

		Compute the elimination tree of a symmetric sparse matrix ``A`` from ``triu(A)`` and, optionally, its post-ordering permutation.

		Note: This function has been moved to the CSparse.jl package.

	#test PkgDev redirects
	begin
	try
	Pkg.register("IDoNotExist")
	error("unexpected")
	catch ex
	@test ex.msg == "Pkg.register(pkg,[url]) has been moved to the package PkgDev.jl.\nRun Pkg.add(\"PkgDev\") to install PkgDev on Julia v0.5-"
	end

	try
	Pkg.tag("IDoNotExist")
	error("unexpected")
	catch ex
	@test ex.msg == "Pkg.tag(pkg, [ver, [commit]]) has been moved to the package PkgDev.jl.\nRun Pkg.add(\"PkgDev\") to install PkgDev on Julia v0.5-"
	end

	try
	Pkg.generate("IDoNotExist","MIT")
	error("unexpected")
	catch ex
	@test ex.msg == "Pkg.generate(pkg, license) has been moved to the package PkgDev.jl.\nRun Pkg.add(\"PkgDev\") to install PkgDev on Julia v0.5-"
	end

	try
	Pkg.publish()
	error("unexpected")
	catch ex
	@test ex.msg == "Pkg.publish() has been moved to the package PkgDev.jl.\nRun Pkg.add(\"PkgDev\") to install PkgDev on Julia v0.5-"
	end

	try
	Pkg.license()
	error("unexpected")
	catch ex
	@test ex.msg == "Pkg.license([lic]) has been moved to the package PkgDev.jl.\nRun Pkg.add(\"PkgDev\") to install PkgDev on Julia v0.5-"
	end
	try
	Pkg.submit("IDoNotExist")
	error("unexpected")
	catch ex
	@test ex.msg == "Pkg.submit(pkg[, commit]) has been moved to the package PkgDev.jl.\nRun Pkg.add(\"PkgDev\") to install PkgDev on Julia v0.5-"
	end
	try
	Pkg.submit("IDoNotExist", "nonexistentcommit")
	error("unexpected")
	catch ex
	@test ex.msg == "Pkg.submit(pkg[, commit]) has been moved to the package PkgDev.jl.\nRun Pkg.add(\"PkgDev\") to install PkgDev on Julia v0.5-"
	end
	end

WIP: move csparse functions to separate package #16070

WIP: move csparse functions to separate package #16070

Conversation

KristofferC commented Apr 27, 2016 • edited by tkelman Loading

tkelman commented Apr 27, 2016 • edited Loading

KristofferC commented Apr 27, 2016

KristofferC commented Apr 27, 2016 • edited Loading

tkelman commented Apr 27, 2016

KristofferC commented Apr 27, 2016

KristofferC Apr 27, 2016

Choose a reason for hiding this comment

tkelman Apr 27, 2016

Choose a reason for hiding this comment

KristofferC Apr 27, 2016

Choose a reason for hiding this comment

tkelman Apr 27, 2016

Choose a reason for hiding this comment

tkelman Apr 27, 2016

Choose a reason for hiding this comment

KristofferC Apr 27, 2016

Choose a reason for hiding this comment

StefanKarpinski commented Apr 27, 2016

tkelman commented Apr 27, 2016

KristofferC commented Apr 27, 2016

tkelman commented Apr 27, 2016 • edited Loading

StefanKarpinski commented Apr 27, 2016

Sacha0 commented Apr 27, 2016

hayd commented Apr 27, 2016

tkelman commented Apr 28, 2016

tkelman commented Apr 30, 2016

KristofferC commented Apr 30, 2016

KristofferC commented May 2, 2016

KristofferC commented May 2, 2016

tkelman commented May 3, 2016 • edited Loading

hayd commented May 3, 2016

tkelman commented May 3, 2016 • edited Loading

KristofferC commented May 3, 2016

tkelman commented May 3, 2016 • edited Loading

KristofferC commented May 14, 2016

KristofferC commented May 14, 2016

tkelman commented May 14, 2016

KristofferC commented May 14, 2016

tkelman commented May 14, 2016

KristofferC commented May 14, 2016

tkelman commented May 14, 2016

tkelman commented May 24, 2016

tkelman commented Jun 4, 2016

ViralBShah commented Jun 4, 2016

tkelman commented Jun 4, 2016 • edited Loading

KristofferC commented Jun 4, 2016

Sacha0 commented Jun 14, 2016

KristofferC commented Jun 14, 2016

hayd commented Jun 14, 2016

tkelman commented Jun 14, 2016 • edited Loading

hayd commented Jun 14, 2016 • edited Loading

tkelman commented Jun 14, 2016

tkelman commented Jun 20, 2016

KristofferC commented Apr 27, 2016 •

edited by tkelman

Loading

tkelman commented Apr 27, 2016 •

edited

Loading

KristofferC commented Apr 27, 2016 •

edited

Loading

tkelman commented Apr 27, 2016 •

edited

Loading

tkelman commented May 3, 2016 •

edited

Loading

tkelman commented May 3, 2016 •

edited

Loading

tkelman commented May 3, 2016 •

edited

Loading

tkelman commented Jun 4, 2016 •

edited

Loading

tkelman commented Jun 14, 2016 •

edited

Loading

hayd commented Jun 14, 2016 •

edited

Loading