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 .sage modules importable #27074

Open
embray opened this issue Jan 18, 2019 · 13 comments
Open

Make .sage modules importable #27074

embray opened this issue Jan 18, 2019 · 13 comments

Comments

@embray
Copy link
Contributor

embray commented Jan 18, 2019

It's relatively easy using import system hooks in Python to make new file types 'importable' using the standard import statement. E.g. a file like mymodule.sage could be imported as mymodule just as though it were mymodule.py.

Importing a .sage module is of course just a thin layer over a normal Python module import, but to pass it through the Sage preparser.

There are of course some risks to making .sage modules importable. For example, what if a user has both a foo.sage and an foo.py? Which one takes priority upon import foo? In particular it could introduce quite some surprises if a user has something like os.sage in their current directory.

Unfortunately it's not possible to extend the syntax of import statements, which would be nice. But, while the "module name" portion of an import statement must be a valid identifier, we can still otherwise process it however we want. So maybe .sage module imports would be initiated only if .sage is used explicitly in the import statement. So for example, to import a sage module named foo.sage one would literally write:

import foo.sage

to distinguish it from a Python module named foo.py. Of course, if there is a Python package named foo that happens to have a sage sub-module there is still a conflict. But this is a bit unlikely for most cases.

So in order to make such a feature available, while mitigating potential problems with it, I might suggest a few additional restrictions:

  1. There would be a function to enable/disable .sage import functionality on the fly. When first introduced this would be disabled by default, but we would certainly want to make the feature easily discoverable through the documentation, with all the caveats discussed.

  2. In the off chance that there is a conflict between a .sage module an a plain Python module, the system should check for that. In that case the .sage module still takes priority (if .sage imports are enabled), but a warning about the conflict is shown.

  3. This feature should be Python 3 only. That will make it easier to implement, due to the significant differences between the import systems, and it will also give a nice motivation, to anyone who wants the feature, to switch to Python 3.

See also: https://groups.google.com/d/msg/sage-devel/tL8Whabyaoc/RMzlMyUtBwAJ

CC: @dimpase @jdemeyer @EmmanuelCharpentier @nthiery @nbruin @yuan-zhou

Component: interfaces

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

@embray embray changed the title Make .sage modules importable Make .sage modules importable Jan 18, 2019
@embray embray self-assigned this Jan 18, 2019
@dimpase
Copy link
Member

dimpase commented Jan 18, 2019

comment:3

Do we need to worry about foo.py shadowing foo.sage? The long-standing situation on this was that foo.sage would generate foo.py - did this change at some point I missed?

@jdemeyer
Copy link
Contributor

comment:4

Replying to @dimpase:

Do we need to worry about foo.py shadowing foo.sage?

I don't see why there is a big worry about this. We just have to decide on an order of trying filenames. This is no different from having a foo.py file, a foo.pyc file, a foo.so and a foo/__init__.py file in the same directory: Python chooses a certain order to try these filenames and it doesn't seem to be a big problem in practice.

So the .sage convention in the import statement seems strange and unneeded to me.

@embray
Copy link
Contributor Author

embray commented Jan 18, 2019

comment:5

Replying to @jdemeyer:

Replying to @dimpase:

Do we need to worry about foo.py shadowing foo.sage?

I don't see why there is a big worry about this. We just have to decide on an order of trying filenames. This is no different from having a foo.py file, a foo.pyc file, a foo.so and a foo/__init__.py file in the same directory: Python chooses a certain order to try these filenames and it doesn't seem to be a big problem in practice.

So the .sage convention in the import statement seems strange and unneeded to me.

Perhaps so. The idea there was just to make it less likely for it to shadow Python modules on sys.path that have absolutely nothing to do with Sage, and cause sudden strange breakage.

But if the other mitigation strategies for that are employed, then perhaps we don't need to use that convention in the first place. Also, many stdlib libraries would likely be imported already by the time the import hooks necessary for supporting .sage imports are loaded. So for example, it's not likely that having an os.sage will cause import os to break because it will have already been imported (the downside to that is that then trying to import their local os.sage will quietly fail, but I think we can still consider this somewhat "intermediate usage" where one has to at least understand what Python imports are and not to name things the same as system libraries...)

@jdemeyer
Copy link
Contributor

comment:6

Replying to @embray:

the downside to that is that then trying to import their local os.sage will quietly fail

Again, this is no different from having an os.py file in the current directory and running import os (technical note: os is a special built-in module which ignores the usual import system).

@defeo
Copy link
Member

defeo commented Jan 18, 2019

comment:7

+100 to having this! Thanks Erik for starting this ticket (it has been on my todo list for a while now).

Replying to @jdemeyer:

Replying to @embray:

the downside to that is that then trying to import their local os.sage will quietly fail

Again, this is no different from having an os.py file in the current directory and running import os (technical note: os is a special built-in module which ignores the usual import system).

I agree with this.

However the ability to import .sage files should be optional, and switchable at runtime, as Erik suggested. Otherwise it could break code that is now working.

Still, if there's foo.py and foo.sage in the same dir, which should shadow which? I'd say foo.py takes priority.

@embray
Copy link
Contributor Author

embray commented Jan 22, 2019

comment:8

Replying to @jdemeyer:

Replying to @embray:

the downside to that is that then trying to import their local os.sage will quietly fail

Again, this is no different from having an os.py file in the current directory and running import os (technical note: os is a special built-in module which ignores the usual import system).

That's true about os; I've seen this more often with things like "string" or "logging".

And I agree it's no different. My only point is that currently it does not work that way for .sage files, so if we implement this it has to be opt-in, at least at first, because suddenly producing side-effects based on files the user happens to have in their working directory is user-hostile.

@embray
Copy link
Contributor Author

embray commented Jan 22, 2019

comment:9

Replying to @defeo:

Still, if there's foo.py and foo.sage in the same dir, which should shadow which? I'd say foo.py takes priority.

It would, just as in Jeroen's analogy to .pyc files. Further, if all of foo.sage, foo.py, and foo.pyc exist it would be the .pyc file that takes precedence. However, if foo.py is newer than foo.pyc it will read that instead and recompile it. I think traditionally the import system would just compare mtimes here, but this is more recently complicated (I think since Python 3.7) by PEP-552. A similar approach could still be taken, if needed, for .sage -> .py compilation.

@embray
Copy link
Contributor Author

embray commented Jun 19, 2019

comment:10

Per discussion with Luca, I found this prototype I worked on in #24681 which might be helpful here: https://gitlab.com/sagemath/dev/trac/compare/8.2.beta4...u%2Fembray%2Fpython3%2Fcython-source-prototype

I remember now that the context to that work was to make the source code for Cython modules findable on Python 3, so the idea was to add an import hook that could recognize imports of Cython-modules based on the presence of the actual .pyx file in the same path as the actual extension module being imported (it does not do anything like automatically interpret/compile the Cython code into binaries; it just recognized Cython modules as distinct from other extension modules).

For importing .sage files the problem is a little different. But all the MetaFileFinder stuff I had to do was not in any way specific to Cython, but rather was necessary in order to extend the default PathFinder installed in sys.meta_hooks on Python 3.

It turns out I didn't implement this for Python 2 after all. As I wrote on #24681 it used to be easier to make import <modname> for non-Python files work, much as in the "shelve importer" example on this pymotw post: https://pymotw.com/2/sys/imports.html#custom-importers I recall once in the past teaching a tutorial on this where I made something similar to import JSON files as Python modules (I'm not sure now where that tutorial is; it was years ago).

I think for now we might as well ignore Python 2. On Python 3 I think it's now a little easier than it used to be to implement custom module loaders. But adding custom module finders is more of a problem. It's a kind of chicken/egg problem, where in Python 2 it was actually easier to override the default behavior with import hooks, because the default behavior was completely hard-coded in the interpreter. But now the default import system is bootstrapped on top of the same import hook system that is used to extend it, with the result that it's now harder to extend the default behavior while not also breaking/contradicting with the default import system.

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Dec 5, 2019

comment:11

[ Sorry for barging in : I saw some interesting light... ]

One of the corollaries of this enhancement is that it should be possible in a similar way to monkey-patch some Sage classes, i. e., for example, adding new methods to Sage-defined classes.

This would considerably ease the prototyping of new extensions (which currently require forking a branch and remakeing it for test.

Would you consider this aspect of the issue in future development ?

@embray
Copy link
Contributor Author

embray commented Dec 6, 2019

comment:13

Replying to @EmmanuelCharpentier:

[ Sorry for barging in : I saw some interesting light... ]

One of the corollaries of this enhancement is that it should be possible in a similar way to monkey-patch some Sage classes, i. e., for example, adding new methods to Sage-defined classes.

This would considerably ease the prototyping of new extensions (which currently require forking a branch and remakeing it for test.

Would you consider this aspect of the issue in future development ?

I'm afraid I don't see at all how that's related. This is just about making .sage files importable through the normal Python import system. Monkey-patching classes is a thing you can already do, having relatively little to do with how modules are imported.

Nicolas worked on something like this with https://github.com/nthiery/recursive-monkey-patch so you might want to ask him about it.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 15, 2020

comment:14

Let's see if we can revive this idea now that the py3 transition is complete...

I think it's a key to the recurring question "how to distribute user packages" -- for user packages that consist of a bunch of sage files

@mkoeppe mkoeppe added this to the sage-9.2 milestone Jul 15, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Aug 13, 2020
@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 13, 2021

comment:18

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Feb 13, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Jul 19, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 14, 2021
@mkoeppe mkoeppe removed this from the sage-9.6 milestone Apr 1, 2022
@mkoeppe mkoeppe added this to the sage-9.7 milestone Apr 1, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Aug 31, 2022
@mkoeppe mkoeppe removed this from the sage-9.8 milestone Jan 29, 2023
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