Readd init files

[tobiasdiez]

There are various issues with using namespace packages because essential dependencies of sage are not fully supporting them. In particular,

• As seen in Use Cython 3 implicit namespace package support #36228 and Compile everything with meson #36524, cython currently doesn't support namespace packages (it creates the wrong module name, perhaps related to [BUG] Building namespace packages does not work with glob patterns cython/cython#5237). Sagelib (more specifically sage-setup) currently implements an ugly workaround by monkey-patching Cython internals
• Sage's on-the-fly cython compilation is currently completely broken as none of the namespace packages can be used in it (see the changed doctest in this PR)
• There are various issues with pytest, some of the are discussed in pytest - fix scope in which doctests are run #33826
• importlib.resources doesn't support namespace packages on Python 3.9. GAP: make GAP_ROOT_PATHS configuration unnecessary #37344 (comment)
• It is not possible to compile and use a cython file using the standard cython cli. This, however, can be very helpful in certain situations where you want to experiment with different cython options but don't want to wait for a complete recompilation of all cython files. I used this quite often while preparing Compile everything with meson #36524.

Until these issues are fixed upstream, we revert #35322 and #35366 by readding the __init__.py files.

The proposed alternative #37009 would only provide an (ugly) workaround for the last issue.

📝 Checklist

• The title is concise, informative, and self-explanatory.
• The description explains in detail what this PR is about.
• I have linked a relevant issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation accordingly.

⌛ Dependencies

[mkoeppe]

This change is not needed in Sage, so why not just keep this workaround in your experimental branch

[tobiasdiez]

This change is not needed in Sage

It fixes the issues you see in #36228. Not sure if you count your PR as being part of Sage or not. This PR also shows that the only reason why its currently working in the develop branch is that sage is monkey patching cython's package discovery.

so why not just keep this workaround in your experimental branch

Because #36524 is already in a good shape (there is only one compilation issue left), so to ease review of it I started to extract changes in that branch to new PRs.

[mkoeppe]

This change is not needed in Sage

It fixes the issues you see in #36228. Not sure if you count your PR as being part of Sage or not. This PR also shows that the only reason why its currently working in the develop branch is that sage is monkey patching cython's package discovery.

Yes, that is precisely the purpose of our customization of Cython.

Obviously, #36228 is not part of Sage. It is a draft PR that investigates what the status of upstream Cython namespace package support is. We cannot yet switch to it.

[tobiasdiez]

We cannot yet switch to it.

Should work with the changes of this PR. Give it a try: mkoeppe#20

[mkoeppe]

Obviously, when you replace namespace packages by ordinary packages, then namespace package support is not needed. What's there to try out.

But we need the namespace packages for the modularization -- which is why we removed the init files.

[mkoeppe]

So why not instead of calling cython from meson, call a custom script that uses our customization.

[tobiasdiez]

Obviously, when you replace namespace packages by ordinary packages, then namespace package support is not needed. What's there to try out.

Matthias, it's not my fault that Cython's namespace package support has bugs. So what's the problem with not using it (for now, until it's ready)?

But we need the namespace packages for the modularization -- which is why we removed the init files.

It runs perfectly fine right now. If additional modularized distributions need namespace support, just don't install the init files (they are needed for compilation only). (Or design around the cython limitation for now and don't use namespace packages.)

So why not instead of calling cython from meson, call a custom script that uses our customization.

I'm not calling cython myself. It's all handled internally by meson, see https://mesonbuild.com/Python-module.html#extension_module.

[mkoeppe]

why not instead of calling cython from meson, call a custom script that uses our customization.

I'm not calling cython myself. It's all handled internally by meson, see https://mesonbuild.com/Python-module.html#extension_module.

Without doubt, trivial to customize.

[mkoeppe]

Cython's namespace package support has bugs. So what's the problem with not using it (for now, until it's ready)?

Once again, Sage is not affected by this bug because we have a working customization of Cython.

[tobiasdiez]

Cython's namespace package support has bugs. So what's the problem with not using it (for now, until it's ready)?

Once again, Sage is not affected by this bug because we have a working customization of Cython.

What's the point of this posturing? Do you just want to make my work on #36524 harder?

Clearly there are bugs in Cython. The options (brought forward so far) are either a) the current approach of monkey patching or b) just not using namespace modules during compilation (i.e. this PR here). Both are workarounds for the same Cython bug. Option a) does not work (easily) for #36524, while b) does work well. So why not go with b)?

[mkoeppe]

What's the point of this posturing? Do you just want to make my work on #36524 harder?

@tobiasdiez I have made clear to you in #36572 (comment) that insinuations of this type are unwelcome. I'm referring this to sage-abuse again.

[dimpase]

nice - so we can revive our pytest efforts, right?

[tornaria]

Shall we set this back to positive review?

No, we shall not.

Why not?

[NathanDunfield]

Why not?

As a disputed PR, it should only be set to positive review if the number of positive votes is at least twice the number of negative votes. I see three negative votes and four positive ones (counting the PR author as positive), though I certainly could have miscounted.

[mkoeppe]

@tornaria I've already explained it in detail in:

• Readd init files #36753 (comment)
• Readd init files #36753 (comment)
• Readd init files #36753 (comment)
• Readd init files #36753 (comment)

[tornaria]

Why not?

As a disputed PR, it should only be set to positive review if the number of positive votes is at least twice the number of negative votes. I see three negative votes and four positive ones (counting the PR author as positive), though I certainly could have miscounted.

Thanks, this was what I meant.

It's very hard to look with all the offtopic; I counted four +1 and two -1, that's why I asked.

[dimpase]

I don't think it humanely possible to count votes this way.
IMHO it should be done by the usual GitHub inteface: Approve/Request chnages. Then it's visible. Right now I see one review from me, comments (I just addressed them) by @saraedum - and that's all.

[saraedum]

@dimpase I agree that the system is not ideal. When you want to set it to positive review, you unfortunately have to browse through the entire PR history to count the votes. I guess we could reviews, somebody could build a vote-counting-bot, or we could use reactions on the PR itself. Anyway, I don't think this is a discussion that should be had on this PR but you could maybe propose something on sage-devel.

So, for the record, currently these are the votes I see. (And hopefully, next time I remember and can start counting from this comment…)

Current Votes (5-3)

👍 @tobiasdiez (author)
👎 @kwankyu #36753 (comment)
👍 @dimpase #36753 (comment)
👎 @mkoeppe #36753 (comment)
👍 @orlitzky #36753 (comment)
👎 @culler #36753 (comment)
👍 @tornaria #36753 (comment)
👍 @saraedum (here)

[mkoeppe]

As a matter of fact, I do encourage individuals in our community to speak up when inappropriate conduct is happening in plain sight.

We welcome all attempts at defusing a debate that has gotten heated. However, if you witness abusive behavior, please be very careful about publicly labeling it as such yourself.

People can speak up without having to label anything. They can simply express that something that they have witnessed makes them uncomfortable. That's already helpful support for targets of abuse.

[jhpalmieri]

I vote -1. I think that makes it 5-4.

[tobiasdiez]

I vote -1. I think that makes it 5-4.

Could you please shortly outline how/why you came to this conclusion? Thanks!

[kwankyu]

I vote -1. I think that makes it 5-4.

Could you please shortly outline how/why you came to this conclusion? Thanks!

It is inappropriate to ask a voter to defend his voting. It is not part of the voting procedure.

[roed314]

It is inappropriate to ask a voter to defend his voting. It is not part of the voting procedure.

I'm just speaking for myself here, not the sage-conduct committee. I don't think it's unreasonable to ask someone for their reasoning, @jhpalmieri is just not obligated to respond.

[tobiasdiez]

I vote -1. I think that makes it 5-4.

Could you please shortly outline how/why you came to this conclusion? Thanks!

It is inappropriate to ask a voter to defend his voting. It is not part of the voting procedure.

Aren't these votes also just normal PR reviews at the same time? According to our documentation, PR reviews needs to be accompanied with a comment explaining what changes you like to see before the PR can be merged.

But more importantly than stressing definitions, I just don't know what I should change in this PR to make it acceptable to the people voting with -1. In particular:

• at Readd init files #36753 (comment), you give the reason for your -1 as "This PR is undoing things done for modularization of the sage library, deliverables of which are distributions packages.". This is simply not true, there is no conflict with the modularization project - all documented ways to test and install the modularizated distributions are working. The "deliverables" are untouched. CI is passing as well. I'm also not aware of any conflicts with other PRs (although I was educated that such conflicts would be meaningless as each PR should only be considered in isolation.)
• at Readd init files #36753 (comment), @culler votes with a -1 because "it does not justify derailing the modularization effort.". Again, there is nothing in this PR that derails the modularization project (or any other project of Matthias or someone else that I'm aware of). And it's definitely not the aim of this PR to derail anything.
• at Readd init files #36753 (comment), @jhpalmieri votes with a -1 without giving any reasoning

So based on reasons given by people with negative votes, what should I change so that this PR can be merged? In particular, where are the "technical discussions" that should happening in these disputed PRs? All I can see agrees with @dimpase's analysis that "people engage in purely political voting here". But please prove me wrong!

[github-actions]

Documentation preview for this PR (built with commit 2e368bc; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[mkoeppe]

People interested in the technical discussion should read #36753 (comment), where I explain step by step why this PR is not a suitable solution.