#24215 closed enhancement (fixed)

Add HAVE_GMPY2 compile-time constant

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.1
Component: cython Keywords:
Cc: vklein Merged in:
Authors: Jeroen Demeyer Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: 7f06e71 (Commits) Commit: 7f06e71efb3fdc5f91fcf4b0b3ef761c72d9a8c2
Dependencies: Stopgaps:

Description

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

Change History (28)

comment:1 follow-ups: Changed 23 months ago by vdelecroix

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.

comment:2 in reply to: ↑ 1 ; follow-ups: Changed 23 months ago by jdemeyer

Replying to vdelecroix:

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?

comment:3 in reply to: ↑ 1 ; follow-up: Changed 23 months ago by jdemeyer

Replying to vdelecroix:

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.

comment:4 in reply to: ↑ 1 Changed 23 months ago by jdemeyer

Replying to vdelecroix:

Not mentioning how impredictable this could be in doctesting.

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

comment:5 in reply to: ↑ 3 ; follow-up: Changed 23 months ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

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?

comment:6 in reply to: ↑ 2 ; follow-up: Changed 23 months ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

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.

comment:7 follow-up: Changed 23 months ago by vklein

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.

comment:8 Changed 23 months ago by vdelecroix

Indeed!

comment:9 Changed 23 months ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/24215

comment:10 in reply to: ↑ 5 Changed 23 months ago by jdemeyer

  • Commit set to 34ef6dc08de5f11a74e83c6707d6dd4f5e2563be

Replying to vdelecroix:

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:

34ef6dcAdd HAVE_GMPY2 compile-time constant

comment:11 Changed 23 months ago by jdemeyer

  • Status changed from new to needs_review

comment:12 in reply to: ↑ 7 Changed 23 months ago by jdemeyer

Replying to vklein:

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.

comment:13 in reply to: ↑ 6 Changed 23 months ago by jdemeyer

Replying to vdelecroix:

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.

comment:14 Changed 23 months ago by vdelecroix

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).

comment:15 Changed 23 months ago by git

  • Commit changed from 34ef6dc08de5f11a74e83c6707d6dd4f5e2563be to 7f06e71efb3fdc5f91fcf4b0b3ef761c72d9a8c2

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

158d984Force absolute import in have_module()
7f06e71Fix documentation

comment:16 in reply to: ↑ 2 Changed 22 months ago by embray

Replying to jdemeyer:

Replying to vdelecroix:

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 :)

comment:17 follow-ups: Changed 22 months ago by embray

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

comment:18 in reply to: ↑ 17 ; follow-up: Changed 22 months ago by vdelecroix

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 14.

comment:19 in reply to: ↑ 17 ; follow-up: Changed 22 months ago by 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.

comment:20 in reply to: ↑ 19 ; follow-up: Changed 22 months ago by embray

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

comment:21 in reply to: ↑ 20 Changed 22 months ago by jdemeyer

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__.

comment:22 in reply to: ↑ 18 ; follow-up: Changed 22 months ago by embray

Replying to vdelecroix:

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 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__.

comment:23 in reply to: ↑ 22 ; follow-up: Changed 22 months ago by jdemeyer

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?

comment:24 in reply to: ↑ 23 ; follow-up: Changed 22 months ago by embray

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__.

comment:25 Changed 22 months ago by embray

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

comment:26 in reply to: ↑ 24 Changed 22 months ago by 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).

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?

comment:27 Changed 22 months ago by embray

  • Reviewers set to Erik Bray
  • Status changed from needs_review to positive_review

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...

comment:28 Changed 22 months ago by vbraun

  • Branch changed from u/jdemeyer/ticket/24215 to 7f06e71efb3fdc5f91fcf4b0b3ef761c72d9a8c2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.