Opened 6 years ago

Closed 6 years ago

#20388 closed defect (fixed)

Fix the Magma interface to work with remote installations

Reported by: Marc Masdeu Owned by:
Priority: major Milestone: sage-7.2
Component: interfaces: optional Keywords: magma, remote
Cc: Merged in:
Authors: Marc Masdeu Reviewers: Nils Bruin, Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: f499648 (Commits, GitHub, GitLab) Commit: f4996488101ac3ce85550fb14b2eec08185408a6
Dependencies: Stopgaps:

Status badges

Description

I have frequently encountered computer setups where one has access to a Sage installation locally, but Magma is only installed in a remote machine (say with address 'remote'). Typically one either: 1) Logs into 'remote' via ssh and does some computation in Magma. 2) uses the ssh command

    $> ssh -t remote 'magma'

to pretend having magma installed locally.

Actually Sage has functionality to use (2) in its interface with Magma, but it is currently broken and not advertised. It presumes that 'remote' also has sage installed (in fact it presumes that sage-native-execute is in the default PATH), for example.

This ticket fixes this, and makes a command like

    sage: magma = Magma(server = 'remote', command = 'magma-myversion')

work, and provide the user with an interface to a particular version of Magma installed in 'remote'.

Change History (47)

comment:1 Changed 6 years ago by Marc Masdeu

Branch: u/mmasdeu/20388
Commit: ec0a724df6548f328d0947ad166c12d96392d74e
Status: newneeds_review

New commits:

8cf2e2aFixed Magma interface to work with remote server.
5fb6df4Fixed spacing.
ec0a724Added message to help in discovering the feature.

comment:2 Changed 6 years ago by Vincent Delecroix

I was able to use it! Note that on the server I have access to I need two commands to launch magma

$ module load magma/2.11.13    # Slurm
$ magma

And Magma(server='plafrim', command='module load magma/2.11.13; magma') worked perfectly. Do you think it is worth it to add in the doc?

Sided note: Would be nice for users to set up their configurations once for all for these external interfaces (via environment variables?). For example, I have access to maple and magma but they live on two different servers.

comment:3 Changed 6 years ago by Marc Masdeu

That's great! I added the 'command' parameter because it could be potentially useful although I didn't need it. I am happy to see that my guess was correct.

About setting defaults, have you tried to put

magma = Magma(server='plafrim', command='module load magma/2.11.13; magma')
maple = (whatever is pertinent, I don't have access to maple)

in $HOME/.sage/init.sage?

Finally, although I agree that the documentation should advertise more this feature, I don't know where to put it exactly.

comment:4 Changed 6 years ago by Vincent Delecroix

Setting them in init.sage is indeed a solution to make magma(something) works out of the box.

My question about default configuration did not make much sense since I thought that SageObject would have methods _magma_ and _maple_. If it was

    def _magma_(self, G=None):
        if G is None:
            import sage.interfaces.magma
            G = sage.interfaces.magma.magma
        return self._interface_(G)

then it would not take into account any specific server configuration.

comment:5 Changed 6 years ago by git

Commit: ec0a724df6548f328d0947ad166c12d96392d74eda0c8a128c83b89f9cc6f42b3dfcdcac3f8fce24

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

da0c8a1Fixed two functions that were using the default magma.

comment:6 Changed 6 years ago by Marc Masdeu

Thanks for testing it! I found two functions (one in arith.all, one in plane_conics/con_field.py) that imported magma. I have changed them to try to use the global magma if it exists (which should be the case if the user initialized it, anyway).

comment:7 Changed 6 years ago by Nils Bruin

A few things you might want to look at:

  • sage's pexpect interfaces have an option to use files for transfer if the input is large. That's going it be an issue, because in your scenario you wouldn't want to assume sage and (remote) magma have access to a shared file system. So you might have to turn this capability off.
  • You're now assuming that the remote magma user has permission to do an "scp" to the remote server. Indeed, the way we currently have written ext/magma requires file access (it's written as a module). However, one could put those files within reach of magma via other means. The "scp" variant possibly works quite often, so it may be a reasonable default thing to try, but we should probably not fail if it doesn't work.

comment:8 Changed 6 years ago by Vincent Delecroix

Relying on globals is not very safe

sage: magma = MyMagmaStructure()
sage: bernoulli(3, algorithm='magma') # boom

I think that the magma interface (and all interface in general) should just have some default configurable parameters. That would be used with something like

sage: sage.interfaces.config('magma', server=X', command='Y')

comment:9 in reply to:  8 Changed 6 years ago by Nils Bruin

Replying to vdelecroix:

I think that the magma interface (and all interface in general) should just have some default configurable parameters. That would be used with something like

sage: sage.interfaces.config('magma', server=X', command='Y')

I would hang those properties on sage.interfaces.magma.magma, which is the "default" instantiation of the magma interface.

Note that sage.interfaces.magma.magma is instantiated upon load, so we're too late to set defaults for its instantiation. However, actually starting up only happens once the interface really gets used, so you could just adapt the relevant properties on the magma object before using the interface.

A workaround you can already do if you want to use a magma with non-standard setting is "monkey patch" the global instance, i.e.,

sage: sage.interfaces.magma.magma = Magma(...<options you want>...)

with the problem that if some other file (such as sage.interfaces.all) has done an import from, you're not reaching them. So perhaps it's a little more robust to adjust attributes on sage.interfaces.magma.magma rather than rebind it to a different instance (sigh ... and then they say you don't have to think about pointers when working in python)

comment:10 Changed 6 years ago by Marc Masdeu

I have addressed Nils' two concerns above. About the "magma = Magma()" and the configuration parameters, I don't have a quick solution yet. There's a bunch of places in the schemes/ folder that import magma depending on the algorithm chosen. One easy way to fix this is to provide a get_magma_session() and set_magma_session() functions so that something like this would work:

sage: set_magma_session(Magma(server = 'remote', command = 'magma_custom'))
sage: function_using_magma(algorithm = 'magma')

This would only involve changing the calls that look like:

from sage.interfaces.magma import magma
magma.eval('complicated stuff')

to

from sage.interfaces.magma import get_magma_session
magma = get_magma_session()
magma.eval('complicated stuff')

Do you think that this is a good enough solution? If so, I would quickly implement it...

comment:11 in reply to:  10 Changed 6 years ago by Vincent Delecroix

Replying to mmasdeu:

I have addressed Nils' two concerns above. About the "magma = Magma()" and the configuration parameters, I don't have a quick solution yet. There's a bunch of places in the schemes/ folder that import magma depending on the algorithm chosen. One easy way to fix this is to provide a get_magma_session() and set_magma_session() functions so that something like this would work:

sage: set_magma_session(Magma(server = 'remote', command = 'magma_custom'))
sage: function_using_magma(algorithm = 'magma')

This would only involve changing the calls that look like:

from sage.interfaces.magma import magma
magma.eval('complicated stuff')

to

from sage.interfaces.magma import get_magma_session
magma = get_magma_session()
magma.eval('complicated stuff')

Do you think that this is a good enough solution? If so, I would quickly implement it...

I do not like it so much. It would involve a lot of changes in the schemes/ folder (that people have maybe already copy/paste in their personal code). And, more importantly, it would be different from any other interface in Sage. What about environment variable?

$ SAGE_MAGMA_COMMAND = "module load magma/2.11.13; magma"
$ SAGE_MAGMA_SERVER = "plafrim"
$ export SAGE_MAGMA_COMMAND SAGE_MAGMA_SERVER
$ sage
...

One just needs in the constructor of Magma something like

class Magma(Interface):
    def __init__(self, server=None, command=None, ...):
        import os
        if server is None:
            server = os.getenv('SAGE_MAGMA_SERVER')
        if command is None:
            command = os.getenv('SAGE_MAGMA_COMMAND')

A solution based on init.sage would not work since it is loaded after anything else.

comment:12 Changed 6 years ago by Marc Masdeu

1) Some users may not be familiar with environment variables. 2) I like to be able to write Sage scripts that work after you start sage. Using environment variables forces you to remember always to set them before you start your script, which can be annoying. 3) The environment variable approach doesn't allow the user to compare the result of executing a function with two different Magma installations. Something like this would be desiderable.

sage: E = EllipticCurve('37a1')
sage: set_magma_session(Magma(command = 'magma_2-18'))
sage: E.analytic_rank(algorithm = 'magma')
0
sage: set_magma_session(Magma(command = 'magma_2-19'))
sage: E.analytic_rank(algorithm = 'magma')
0

4) I don't understand your first comment. If people has taken Sage code that uses Magma and now Sage adds more functionality, then they can either use the old functionality or adapt the code to the new (and better way), right?

Here is what I propose: add the environment variables (which make sense the moment there is a magma = Magma() line in there...), but also change the functions using magma to use the get_magma_session() function. How reasonable is this?

comment:13 in reply to:  12 Changed 6 years ago by Vincent Delecroix

Replying to mmasdeu:

Here is what I propose: add the environment variables (which make sense the moment there is a magma = Magma() line in there...), but also change the functions using magma to use the get_magma_session() function. How reasonable is this?

You are right that it is desirable to have a way to modify it from Sage itself. However, I would not populate the global namespace with get_magma_session unless the global magma is deprecated (which can be done using lazy imports).

Moreover, it is very confusing to have all of: magma, Magma, magma_free, magma_console, magma_version in the global namespace... (not talking about Magmas). A cleanup is desirable.

comment:14 Changed 6 years ago by Nils Bruin

I think the set_magma_session is a rather heavy interface. It seems to me the appropriate place would be a method on sage.interfaces.expect.Expect along the lines of

def set_server_and_command(self,server,command):
    if self._expect:
        raise RuntimeError("interface has already started")
    self._server = server
    self.__command = command

If there are other settings to change as well, it may be necessary to override this routine on Magma (and do the usual super() call thing)

The main thing is that an instantiated expect interface doesn't run a process on its own. Starting it is a separate operation (and in fact, during the lifetime of an expect object it may start and stop underlying processes repeatedly)

The cleaner solution would be to make new Magma instances for different server/command settings, but as we've seen a lot of code assumes that there is a single, global, magma instance for default use throughout the lifetime of a sage session.

comment:15 in reply to:  14 Changed 6 years ago by Vincent Delecroix

Replying to nbruin:

I think the set_magma_session is a rather heavy interface. It seems to me the appropriate place would be a method on sage.interfaces.expect.Expect along the lines of

def set_server_and_command(self,server,command):
    if self._expect:
        raise RuntimeError("interface has already started")
    self._server = server
    self.__command = command

I like better this approach. That way the server can even be configured in init.sage!

The cleaner solution would be to make new Magma instances for different server/command settings, but as we've seen a lot of code assumes that there is a single, global, magma instance for default use throughout the lifetime of a sage session.

I would actually remove the Magma from the global namespace. That would not prevent anyone from testing different versions.

comment:16 Changed 6 years ago by Marc Masdeu

Branch: u/mmasdeu/20388u/mmasdeu/20388-fix
Commit: da0c8a128c83b89f9cc6f42b3dfcdcac3f8fce24a8ee09a95a6bf7b80564dd644c7d82b609b790c7

New commits:

d4c1650Disallowed file transfers when using remote Magma. Added error checking
d85b390Added possibility of changing Magma defaults via environment variables.
a8ee09aAdded functionality in expect.py to set the server and command.

comment:17 Changed 6 years ago by Vincent Delecroix

Tested succesfully with magma and matlab! Great.

Though, there is something wrong with maple (not sure where and how it might be related to the changes in this ticket)

sage: maple.set_server_and_command('calcul1')
sage: maple('2+2')  # hang out infinitely

And even after a Ctrl-C I do not have back the sage prompt (only ^CInterrupting Maple...). After a second Ctrl-C I get the prompt back with the following traceback

...
.../interfaces/maple.pyc in set(self, var, value)
    619         """
    620         cmd = '%s:=%s:' % (var, value)
--> 621         out = self.eval(cmd)
    622         if out.find("error") != -1:
    623             raise TypeError("Error executing code in Maple\nCODE:\n\t%s\nMaple ERROR:\n\t%s" % (cmd, out))

.../interfaces/expect.pyc in eval(self, code, strip, synchronize, locals, allow_use_file, split_lines, **kwds)
   1258                 elif split_lines:
   1259                     return '\n'.join([self._eval_line(L, allow_use_file=allow_use_file, **kwds)
-> 1260                                         for L in code.split('\n') if L != ''])
   1261                 else:
   1262                     return self._eval_line(code, allow_use_file=allow_use_file, **kwds)

.../interfaces/maple.pyc in _eval_line(self, line, allow_use_file, wait_for_prompt, restart_if_needed)
    571         with gc_disabled():
    572             z = Expect._eval_line(self, line, allow_use_file=allow_use_file,
--> 573                     wait_for_prompt=wait_for_prompt).replace('\\\n','').strip()
    574             if z.lower().find("error") != -1:
    575                 raise RuntimeError("An error occurred running a Maple command:\nINPUT:\n%s\nOUTPUT:\n%s" % (line, z))
...

(and maple works perfectly well on calcul1 through ssh).

comment:18 Changed 6 years ago by Vincent Delecroix

Might be too much, but we could even move the environment part inside pexpect

server = os.getenv('SAGE_{}_SERVER'.format(self.name().upper()))

What do you think?

To fit with coding conventions could change

(self,server = None,command = None, server_tmpdir = None, ulimit = None)
-->
(self, server=None, command=None, server_tmpdir=None, ulimit=None)

And add cross doctests between set_server_and_command, server and command.

comment:19 Changed 6 years ago by Vincent Delecroix

An annoying feature

sage: magma.set_<TAB>
Creating list of all Magma intrinsics for use in tab completion.
...
Scanning Magma types ...
Last edited 6 years ago by Vincent Delecroix (previous) (diff)

comment:20 Changed 6 years ago by git

Commit: a8ee09a95a6bf7b80564dd644c7d82b609b790c78a28c1a382438c330b8a17297961605f4f3cd9c8

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

8a28c1aAdded examples. Moved environment variables to expect.

comment:21 in reply to:  19 Changed 6 years ago by Marc Masdeu

Replying to vdelecroix:

An annoying feature

sage: magma.set_<TAB>
Creating list of all Magma intrinsics for use in tab completion.
...
Scanning Magma types ...

This only happens the first time you use a server/tmpdir (as long as tmpdir is not cleaned). It is part of the fun to be able to use the discovery feature of python with Magma! So this should be seen as a nice feature, and it was done in your computer the first time you used the magma session as well...

comment:22 in reply to:  17 ; Changed 6 years ago by Marc Masdeu

Replying to vdelecroix:

Though, there is something wrong with maple (not sure where and how it might be related to the changes in this ticket)

I have no easy way to debug this. Maybe open a new ticket?

comment:23 in reply to:  18 Changed 6 years ago by Marc Masdeu

Done!

Replying to vdelecroix:

Might be too much, but we could even move the environment part inside pexpect

server = os.getenv('SAGE_{}_SERVER'.format(self.name().upper()))

What do you think?

To fit with coding conventions could change

(self,server = None,command = None, server_tmpdir = None, ulimit = None)
-->
(self, server=None, command=None, server_tmpdir=None, ulimit=None)

And add cross doctests between set_server_and_command, server and command.

comment:24 in reply to:  22 Changed 6 years ago by Vincent Delecroix

Replying to mmasdeu:

Replying to vdelecroix:

Though, there is something wrong with maple (not sure where and how it might be related to the changes in this ticket)

I have no easy way to debug this. Maybe open a new ticket?

It is now working (with and without your branch)... strange.

comment:25 Changed 6 years ago by Vincent Delecroix

We have a strange message before the Sage prompt if we do not specify the tmp repository

$ export SAGE_MAPLE_SERVER='calcul1'
$ sage
SageMath version ...
No remote temporary directory (option server_tmpdir) specified,
using /tmp/ on calcul1
sage: 

Do you think this is ok? Everything is fine with

$ export SAGE_MAPLE_SERVER='calcul1'
$ export SAGE_MAPLE_TMPDIR='/tmp'
$ sage

comment:26 Changed 6 years ago by Marc Masdeu

Yes, if you don't specify the tmpdir, then this message gets printed (this was already there when I looked at the code). You can specify the tmpdir either in the environment (as you do) or from within the set_server_and_command method.

comment:27 Changed 6 years ago by Vincent Delecroix

Replying to mmasdeu:

Replying to vdelecroix:

An annoying feature

sage: magma.set_<TAB>
Creating list of all Magma intrinsics for use in tab completion.
...
Scanning Magma types ...

This only happens the first time you use a server/tmpdir (as long as tmpdir is not cleaned). It is part of the fun to be able to use the discovery feature of python with Magma! So this should be seen as a nice feature, and it was done in your computer the first time you used the magma session as well...

Not exactly, it is done the first time you use tab completion with magma. And the message says that it is saved locally in .sage//magma_intrinsic_cache.sobj (with two ugly slashes). Might be better to make it parameters dependent because two magma versions might have different namespaces, no? (anyway not for this ticket)

comment:28 Changed 6 years ago by Vincent Delecroix

Sided remark: there is a .format() trick to deal with curly brackets. Double brackets are transformed into one

sage: 'SAGE_{{{{}}}}_{{}}_{}'.format('one').format('two').format('three')
'SAGE_three_two_one'
sage: '{{{}}}'.format(1)
'{1}'

comment:29 Changed 6 years ago by Vincent Delecroix

As you moved the code of magma_version into the class, you should deprecate it (since it is in the global namespace).

comment:30 Changed 6 years ago by Vincent Delecroix

Status: needs_reviewneeds_work

In the Magma constructor there is the default option command = 'magma' which prevents using the environment variable SAGE_MAGMA_COMMAND.

comment:31 Changed 6 years ago by Vincent Delecroix

Setting the default to None is incompatible with

        if not user_config:
            command += ' -n'

which results in

TypeError: unsupported operand type(s) for +=: 'NoneType' and 'str'

comment:32 Changed 6 years ago by git

Commit: 8a28c1a382438c330b8a17297961605f4f3cd9c831977871ebc1d25677951f8cbbe6dee3105c9868

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

3197787Fixed user_config and environment variable clash.

comment:33 Changed 6 years ago by Marc Masdeu

I fixed it by (also) reading the environment variable in magma.py. Each interface can do something similar if needed.

comment:34 in reply to:  27 ; Changed 6 years ago by Nils Bruin

Replying to vdelecroix:

Not exactly, it is done the first time you use tab completion with magma. And the message says that it is saved locally in .sage//magma_intrinsic_cache.sobj (with two ugly slashes). Might be better to make it parameters dependent because two magma versions might have different namespaces, no? (anyway not for this ticket)

No, that is the best place to save it because that's one of the few (hopefully persistent) places where you might expect to have write access. Two magma versions tend to have only subtly different namespaces by default, so the enormous cost of trying to differentiate is probably not offset by having slightly more accurate tab completion.

comment:35 Changed 6 years ago by Marc Masdeu

Status: needs_workneeds_review

comment:36 Changed 6 years ago by git

Commit: 31977871ebc1d25677951f8cbbe6dee3105c9868ce26d46998e1c1be16babab4225dc0f9e1dac281

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

ce26d46Removed command_args parameter from one place.

comment:37 in reply to:  34 Changed 6 years ago by Vincent Delecroix

Replying to nbruin:

Replying to vdelecroix:

Not exactly, it is done the first time you use tab completion with magma. And the message says that it is saved locally in .sage//magma_intrinsic_cache.sobj (with two ugly slashes). Might be better to make it parameters dependent because two magma versions might have different namespaces, no? (anyway not for this ticket)

No, that is the best place to save it because that's one of the few (hopefully persistent) places where you might expect to have write access. Two magma versions tend to have only subtly different namespaces by default, so the enormous cost of trying to differentiate is probably not offset by having slightly more accurate tab completion.

I was not complaining about the location, just about potential version clashes. However, this seems to be completely broken (at least with magma 2.11.13 on my computer).

comment:38 Changed 6 years ago by Vincent Delecroix

Status: needs_reviewneeds_work

Marc, you did not address 29, see Deprecation in the developer guide.

comment:39 Changed 6 years ago by git

Commit: ce26d46998e1c1be16babab4225dc0f9e1dac281c079a14424e9a1ea77179c658284b867b8401ef8

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

c079a14Deprecated and removed from global namesplace the function magma_version.

comment:40 Changed 6 years ago by Marc Masdeu

Status: needs_workneeds_review

I have deprecated the function magma_version. When calling it, you get now an explanatory message. I get a ton of other deprecation warnings which I think are not my fault, so I hope that they don't delay the review of this ticket!

comment:41 Changed 6 years ago by Vincent Delecroix

Replying to mmasdeu:

I have deprecated the function magma_version. When calling it, you get now an explanatory message. I get a ton of other deprecation warnings which I think are not my fault, so I hope that they don't delay the review of this ticket!

Nope. There is another ticket which actually tracks the issue. This is a problem with the way Sage uses Ipython.

comment:42 Changed 6 years ago by Vincent Delecroix

Technically, you deprecated the import in the global namespace but not the function in the module. In other words, this does not raise a warning as it should

sage: sage.interfaces.magma.magma_version()

(because in the long run, we should just get rid of magma_version). Would be better to simply deprecate magma_version and not modify all.py using

def magma_version():
    from sage.misc.superseded import deprecation
    deprecation(20388, 'whatever')
    return magma.version()

Other than that, everything looks good to me. Nils?

comment:43 Changed 6 years ago by git

Commit: c079a14424e9a1ea77179c658284b867b8401ef8f4996488101ac3ce85550fb14b2eec08185408a6

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

6c7236eRevert "Deprecated and removed from global namesplace the function magma_version."
f499648Deprecated magma_version instead of its importing.

comment:44 Changed 6 years ago by Vincent Delecroix

This good for me, only needs Nils agreement. Thanks for the fix. Possible follow-up: #13885

comment:45 Changed 6 years ago by Nils Bruin

I don't have an axe to grind on this. My general impression is that the proposal here should improve the status quo.

comment:46 Changed 6 years ago by Vincent Delecroix

Reviewers: Nils Bruin, Vincent Delecroix
Status: needs_reviewpositive_review

comment:47 Changed 6 years ago by Volker Braun

Branch: u/mmasdeu/20388-fixf4996488101ac3ce85550fb14b2eec08185408a6
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.