Add HAVE_GMPY2 compile-time constant

[jdemeyer]

In order to optionally include code depending on gmpy2, we introduce a compile-time Cython constant HAVE_GMPY2.

CC: @vinklein

Component: cython

Author: Jeroen Demeyer

Branch/Commit: 7f06e71

Reviewer: Erik Bray

Issue created by migration from https://trac.sagemath.org/ticket/24215

[videlec]

comment:1

I find this too complicated. Having Sage compilation depending on optional packages is awful.

For example, if I want to compile Sage with gmpy2 support I would have to do:

1. compile Sage
2. install gmpy2
3. recompile Sage

Not mentioning how impredictable this could be in doctesting.

[jdemeyer]

comment:2

Replying to @videlec:

I find this too complicated. Having Sage compilation depending on optional packages is awful.

We already do that for several packages (e.g. bliss, coxeter3) and it works fine. What is the problem really?

[jdemeyer]

comment:3

Replying to @videlec:

For example, if I want to compile Sage with gmpy2 support I would have to do:

1. compile Sage
2. install gmpy2
3. recompile Sage

Two steps are sufficient:

1. Install gmpy2
2. Compile Sage

Again, this is not different from existing optional packages.

[jdemeyer]

comment:4

Replying to @videlec:

Not mentioning how impredictable this could be in doctesting.

Use # optional - gmpy2. Again, this is not different from existing optional packages.

[videlec]

comment:5

Replying to @jdemeyer:

Replying to @videlec:

For example, if I want to compile Sage with gmpy2 support I would have to do:

1. compile Sage
2. install gmpy2
3. recompile Sage

Two steps are sufficient:

1. Install gmpy2
2. Compile Sage

Again, this is not different from existing optional packages.

sage -i gmpy2 before make?

[videlec]

comment:6

Replying to @jdemeyer:

Replying to @videlec:

I find this too complicated. Having Sage compilation depending on optional packages is awful.

We already do that for several packages (e.g. bliss, coxeter3) and it works fine. What is the problem really?

Not exactly. These two examples are handled as optional extensions. It is easy to figure out what is actually changed by their installation. With compilation constant, the distinction is less clear.

[vinklein]

comment:7

Just as a reminder trac #23024 require pplpy and therefore gmpy2 to be standard package. I am aware that it's not directly linked to the subject but that means we are probably going to make gmpy2 optional then standard again.

[videlec]

comment:8

Indeed!

[jdemeyer]

Branch: u/jdemeyer/ticket/24215

[jdemeyer]

comment:10

Replying to @videlec:

sage -i gmpy2 before make?

Yes, that is what I meant. Although in practice most people will install Sage first and then optional packages.

---

New commits:

34ef6dc

Add HAVE_GMPY2 compile-time constant

[jdemeyer]

Commit: 34ef6dc

[jdemeyer]

comment:12

Replying to @vinklein:

that means we are probably going to make gmpy2 optional then standard again.

Fine for me. To be clear: I'm not opposing to make gmpy2 a standard package. But I am opposed to make it standard now just because it will needed by some hypothetical feature in the future.

[jdemeyer]

comment:13

Replying to @videlec:

With compilation constant, the distinction is less clear.

If you look at the code on #22928, it's really not so bad. Additionally, the HAVE_GMPY2 constant is something that you can easily grep for to find out what depends on gmpy2.

[videlec]

comment:14

Your have_module function modifies the environment

sage: import sys
sage: 'scipy' in sys.modules
False
sage: have_module('scipy')
True
sage: 'scipy' in sys.modules
True

(see also #20382 comment 40). It would better be specified in the doc. Moreover, the function is not testing that a module is installed but testing whether you can load a given module (that might well correspond to a file in the current directory). According to the specification, this is one false positive (a module not installed) and a wrong negative (a module installed but that can not be loaded).

[sagetrac-git]

Changed commit from 34ef6dc to 7f06e71

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

158d984	Force absolute import in have_module()
7f06e71	Fix documentation

[embray]

comment:16

Replying to @jdemeyer:

Replying to @videlec:

I find this too complicated. Having Sage compilation depending on optional packages is awful.

We already do that for several packages (e.g. bliss, coxeter3) and it works fine. What is the problem really?

If anything Sage needs more of this sort of thing, with more packages made "optional" IMO :)

[embray]

comment:17

I don't understand why this calls __import__ with a bunch of a default arguments instead of just using importlib.import_module.

[videlec]

comment:18

Replying to @embray:

I don't understand why this calls __import__ with a bunch of a default arguments instead of just using importlib.import_module.

To avoid local imports. See comment:14.

[jdemeyer]

comment:19

Replying to @embray:

I don't understand why this calls __import__ with a bunch of a default arguments instead of just using importlib.import_module.

importlib.import_module is almost undocumented, so I'm not convinced that it does the right thing.

On the other hand, __import__ has ample documentation so I know that it does what I want it to do.

[embray]

comment:20

Replying to @jdemeyer:

Replying to @embray:

I don't understand why this calls __import__ with a bunch of a default arguments instead of just using importlib.import_module.

importlib.import_module is almost undocumented, so I'm not convinced that it does the right thing.

On the other hand, __import__ has ample documentation so I know that it does what I want it to do.

See https://docs.python.org/3.6/library/importlib.html#importlib.import_module

[jdemeyer]

comment:21

Replying to @embray:

See https://docs.python.org/3.6/library/importlib.html#importlib.import_module

And how does it handle relative imports in Python 2? That's not documented, but it is documented for __import__.

[embray]

comment:22

Replying to @videlec:

Replying to @embray:

I don't understand why this calls __import__ with a bunch of a default arguments instead of just using importlib.import_module.

To avoid local imports. See comment:14.

If I understand your point correctly, using __import__ does not address the problem you're referring to in that comment. E.g.

>>> import importlib, sys
>>> 'numpy' in sys.modules
False
>>> mod = __import__('numpy', {}, {}, [], 0)
>>> mod
<module 'numpy' from '/usr/lib/python2.7/site-packages/numpy/__init__.pyc'>
>>> 'numpy' in sys.modules
True

This is because (traditionally) inserting the module into sys.modules is a job performed by the relevant module Loader. The fact that that happens at all is in a lower level of abstraction than the import statement (which calls __import__).

importlib.import_module is a wrapper around __import__ that just has more obvious semantics in terms of how dotted module names are handled, and is all around simpler. The Python 3 docs recommend using it for this kind of purpose over __import__.

[jdemeyer]

comment:23

Replying to @embray:

The Python 3 docs

...are not relevant since Sage still uses Python 2.

What are you pushing so much for importlib.import_module? If __import__ works, why shouldn't we use it?

[embray]

comment:24

Replying to @jdemeyer:

Replying to @embray:

The Python 3 docs

...are not relevant since Sage still uses Python 2.

For now...

What are you pushing so much for importlib.import_module? If __import__ works, why shouldn't we use it?

I'm so sorry that the documentation in Python 2 surrounding this is not to your liking. A lot of the import-related modules that wasn't previously well documented is better documented now in the Python 3 docs simply because that's where the documentation improvements were made, and many of them were not backported I guess (maybe in part because there were so many other changes to the import system).

The whole point of import_module is that it's just simpler and implements to "obvious" semantics of import-by-name. With __import__ you pass in a mysterious {} basically to to force absolute imports, where as with import_module it's plain: import_module('numpy').

In other words, its purpose is to do exactly what you want to do without dealing with the more complicated (and sometimes shifting) sematics of __import__.

[embray]

comment:25

This is kind of like asking "why are you using MyClass() instead of inst = MyClass.__new__(); if isinstance(inst, MyClass): inst.__init__()?"

[jdemeyer]

comment:26

Replying to @embray:

I'm so sorry that the documentation in Python 2 surrounding this is not to your liking.

You're sounding sarcastic (intentional or not, I have no idea).

But the point is that documentation is important. The import statement maps directly to __import__ and __import__ is well documented. So I can be confident with using __import__ here.

If you want to use importlib and want me to review that, then I will need to read the source code of importlib. That is not a disaster, but why should I be forced to do that?

[embray]

comment:27

Replying to @jdemeyer:

Replying to @embray:

I'm so sorry that the documentation in Python 2 surrounding this is not to your liking.

You're sounding sarcastic (intentional or not, I have no idea).

Yes and no. I sincerely am sorry, because I understand where you're coming from and I don't think this actually matters much, but I also feel like you're stubbornly sticking to a more complicated way to do a thing because the version of the documentation you looked at wasn't as current-as if documentation is always perfect. In this case your choice isn't wrong--it's just needlessly using a lower-level interface for something that can be doing with a simpler higher level interface. importlib.import_module was explicitly backported to Python 2.7 to help write more forward-compatible code.

But the point is that documentation is important. The import statement maps directly to __import__ and __import__ is well documented. So I can be confident with using __import__ here.

If you want to use importlib and want me to review that, then I will need to read the source code of importlib. That is not a disaster, but why should I be forced to do that?

No, actually, you don't; not any more than you need to read the source code of __import__ itself.

I'll explain--I think the misunderstanding here is that importlib was originally added in Python 3, and small parts of it backported to Python 2.7 to aid with forward compatibility. So in fact this function does work the same as documented in the current Python 3 docs (though it's implemented slightly differently). The admonition in the more current docs still applies: "Note: [__import__] is an advanced function that is not needed in everyday Python programming, unlike importlib.import_module()."

By all means, leave it as is--as you said it works. I was just trying to help by pointing out a simpler way...

[embray]

Reviewer: Erik Bray

[vbraun]

Changed branch from u/jdemeyer/ticket/24215 to 7f06e71