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

Make export list explicit instead of re-exporting Hecke++ #2851

Open
benlorenz opened this issue Sep 25, 2023 · 11 comments
Open

Make export list explicit instead of re-exporting Hecke++ #2851

benlorenz opened this issue Sep 25, 2023 · 11 comments

Comments

@benlorenz
Copy link
Member

I think we should make the exports of Oscar explicit instead of exporting (almost) everything from Hecke++ automatically.

I guess we could autogenerate a list once and maintain that but we cannot really manage breaking and non-breaking releases for Oscar when we re-export (almost) everything from Hecke, Nemo, and AbstractAlgebra.

Oscar.jl/src/imports.jl

Lines 176 to 180 in aa8e64f

for i in names(Hecke)
(i in exclude_hecke || !isdefined(Hecke, i)) && continue
@eval import Hecke: $i
@eval export $i
end

https://github.com/thofma/Hecke.jl/blob/75dbabf5ab7c863bba8fc233a85583b674211554/src/Hecke.jl#L99-L103
https://github.com/Nemocas/Nemo.jl/blob/master/src/Nemo.jl#L58-L64

@thofma
Copy link
Collaborator

thofma commented Sep 25, 2023

Can you elaborate why we cannot manage breaking/non-breaking releases of Oscar?

@benlorenz
Copy link
Member Author

I think it is hard to keep track of our versions like that. Right now we should probably bump the Oscar version to 0.14 since Hecke, Nemo, and AbstractAlgebra had their minor versions bumped and we are re-exporting code that was marked as breaking in those packages.

@benlorenz
Copy link
Member Author

I was also confused by the removal of an export in #2846 which turns out to have no effect due to the automatic re-export.

@lgoettgens
Copy link
Member

I totally agree with your reasoning. However, I see some downpoints in doing that:

  1. It would seem kind of arbitrary to me to only do explicit im/exports for Hecke but not for Nemo and ÄA. So, something similar should be between these as well. (a lot of initial work)
  2. When we add a new function to, say, AA, one would need to adapt Nemo and Hecke as well to make that function available in Oscar (assuming 1.). This seems like a large hurdle with lots of PRs and waiting for releases.

As an alternative to the problem in 2. we could import AA functions directly, instead of piping everything from AA using im/exports through Nemo and Hecke. Nemo/Hecke need only to import them themselves, if they want to extend it (if they then should re-export it is a different discussion).

@lkastner
Copy link
Member

  1. It would seem kind of arbitrary to me to only do explicit im/exports for Hecke but not for Nemo and ÄA. So, something similar should be between these as well. (a lot of initial work)

These would then be issues for Nemo/Hecke, but we are dealing with Oscar here.

@lgoettgens
Copy link
Member

I think it is hard to keep track of our versions like that. Right now we should probably bump the Oscar version to 0.14 since Hecke, Nemo, and AbstractAlgebra had their minor versions bumped and we are re-exporting code that was marked as breaking in those packages.

After thinking again about this, I don't think that your proposal doesn't change matters for this problem. Oscar has a lot of functions from AA/Nemo/Hecke that only get re-exported without adding any new methods to them. I see two ways for dependents of Oscar to handle these:

  1. Declare their own compat on AA/Nemo/Hecke and keep track of their breaking releases and compatabilities. I don't think that this is what we want for our users, as otherwise we wouldn't need to re-export anything because users can type using AbstractAlgebra, Nemo, Hecke, Oscar instead of using Oscar themselves, too.
  2. We guarantee the breakiness (is this even a word?) of Oscar even for the case that a dependent doesn't declare compats with AA/Nemo/Hecke. But then, any breaking release of AA/Nemo/Hecke is breaking for oscar as well.

@benlorenz
Copy link
Member Author

benlorenz commented Sep 25, 2023

2. But then, any breaking release of AA/Nemo/Hecke is breaking for oscar as well.

Yeah I guess strictly speaking that is correct even with the list but then we would at least notice removed exports (via Aqua).
And with a list we could also compare the functions with the changelog of the packages.

@lkastner
Copy link
Member

lkastner commented Sep 25, 2023

Either way, the imports need to be cleaned up. Currently we also import a lot of functions manually from AA, Hecke, Nemo that are not necessary due to these loops. (Some of these are necessary, since they are not exported in AA/Hecke/Nemo.) So if the loops stay, these should be removed.

Furthermore the loops make developing harder. If I debug something that uses a function coming from AA, I might not even be able to figure out that it comes from there. Let's say I have code using weak_popov_with_transform from AA. I can find it in Oscar docs, but the search already does not autocomplete it and there is no source link to click on to get to AA. This is not a new issue: #643 . The user might even not run the docs search. I am sure that there are mechanisms in Julia that allow me to work around this, my main point is simply that this makes it harder than necessary. If I use Julia I might e.g. have to determine the exact signature to find the right variant.

@fieker
Copy link
Contributor

fieker commented Sep 26, 2023

Why is this now coming up? Is there a real, existing problem that we're trying to solve? If not: we cannot affort to redo basics just because now we would have done them better - or just differently.
Finding stuff in Oscar is hard due to the many packages involved, here @which, @less, methods, ... are your friends - or git grep in the repos.
There is no real future prospect of ever being able to to using AbstractAlgebra, Oscar at all: AA is exporting, for "good" reason stuff that we do not want to have in Oscar and will produce a name clash (eg. ZZ, QQ)
Hecke, on the other hand was conceived to be a subset of Oscar. I cannot reall see anything exported in Hecke that I don't want to see in Oscar. Nemo is borderline.

But seriously: why?

To find weak_popov_with_transform: methods(Oscar.weak_popov_with_transform) anything else is hard:

julia> methods(Oscar.weak_popov_with_transform) 
# 1 method for generic function "weak_popov_with_transform" from AbstractAlgebra:
 [1] weak_popov_with_transform(A::MatElem{T}) where T<:PolyRingElem
     @ ~/.julia/dev/AbstractAlgebra/src/Matrix.jl:5494

The argument with "do not now where it's from" is totally moot as this would ONLY cover names introduced and methods provided only in AA (Nemo, ..) any name that is filled in many packages ("*") you'll have to dig and hope anyway that is the price of multiple dispatch.

To come back: in Hecke, possibly with the exception of s.th. really fancy, every export should be exported in Oscar.
In Nemo: same is (mostly) true - we can debate some functions, but they have been with us for ever
in AA: cannot be done completely automatically: clashes

Who is going to maintain the synced import/ export lists across all packages?

Unless there is a real benefit, I vote to not change this pattern. It works.

@benlorenz
Copy link
Member Author

benlorenz commented Sep 26, 2023

Unless there is a real benefit, I vote to not change this pattern. It works.

The benefit would be stability for users of Oscar. That was the main point in my original post.

  • We have been putting quite some effort into managing patch-releases and minor releases and taking care of deprecations, making sure we don't break existing syntax. Exactly this is what triggered this discussion since @lgoettgens removed an explicit export dense_row without deprecation or bump.
  • But on the other hand any bump to the minor version of AbstractAlgebra, Nemo and Hecke (that we were doing quite liberally, which I don't think is a bad thing per se) is allowed to make breaking changes to many exported functions.

Right now we have a jump from Hecke 0.20 to 0.22 queued for a 0.13.1 release, Nemo 0.35 to 0.36
and AbstractAlgebra 0.31 to 0.32.

So if we want to keep the development model in the current form I think we should not do patch-releases on master (or at least bump the minor version for every minor-version bump of Hecke, Nemo, or AbstractAlgebra). Current master should be 0.14.0-DEV and 0.13.1 will only be done if someone wants to backport some bugfixes to a separate branch.

@fieker
Copy link
Contributor

fieker commented Sep 28, 2023

Too many packages, the usual problem

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

5 participants