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

improve and document GAP and libgap memory customisation #34041

Closed
dimpase opened this issue Jun 21, 2022 · 20 comments
Closed

improve and document GAP and libgap memory customisation #34041

dimpase opened this issue Jun 21, 2022 · 20 comments

Comments

@dimpase
Copy link
Member

dimpase commented Jun 21, 2022

it's a FAQ, how to do this. Should be fixed, and a working solution provided.

one can test this by exporting env.vars
SAGE_GAP_MEMORY and/or SAGE_GAP_COMMAND (details in the branch`), before testing, e.g.

 $ export SAGE_GAP_MEMORY="500m"
 $ make test

CC: @kiwifb

Component: interfaces

Author: Dima Pasechnik

Branch/Commit: e6ef4fe

Reviewer: Matthias Koeppe

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

@dimpase dimpase added this to the sage-9.7 milestone Jun 21, 2022
@dimpase
Copy link
Member Author

dimpase commented Jun 22, 2022

Author: Dima Pasechnik

@dimpase
Copy link
Member Author

dimpase commented Jun 22, 2022

New commits:

b914324allow overriding GAP memory defaults via env.vars

@dimpase
Copy link
Member Author

dimpase commented Jun 22, 2022

@dimpase
Copy link
Member Author

dimpase commented Jun 22, 2022

Commit: b914324

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 22, 2022

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

6a2dcc0fix docs rst syntax

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 22, 2022

Changed commit from b914324 to 6a2dcc0

@dimpase

This comment has been minimized.

@mkoeppe
Copy link
Member

mkoeppe commented Jun 25, 2022

comment:4

-1 on using os.getenv directly; this should go through sage.env

@mkoeppe
Copy link
Member

mkoeppe commented Jun 25, 2022

comment:5

Also, the naming scheme used by sage.interfaces gives SAGE_GAP_COMMAND, not SAGE_PEXPECT_GAP_COMMAND, see #33405

@dimpase
Copy link
Member Author

dimpase commented Jun 25, 2022

comment:7

Replying to @mkoeppe:

-1 on using os.getenv directly; this should go through sage.env

downstreams don't like sage.env, do they?

@mkoeppe
Copy link
Member

mkoeppe commented Jun 25, 2022

comment:8

The module sage.env is our general interface to access such variables

@kiwifb
Copy link
Member

kiwifb commented Jun 25, 2022

comment:9

I love sage.env. In fact, I had my own equivalent of sage.env in sage-on-gentoo before there was one in vanilla sage. Of course that doesn't mean you should put anything and everything in there.

Now, I am not really against using os.getenv directly in code to change a behavior on the fly. But if you do, it should fall back to a sensible default or nothing if it is not defined. sage.env arguably serves as a single stop to collect such variables that ensure consistent use. But I wouldn't be to bothered if the branch was going as is.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 29, 2022

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

e6ef4feuse sage.env, SAGE_PEXPECT_.. -> SAGE_GAP_COMMAND

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 29, 2022

Changed commit from 6a2dcc0 to e6ef4fe

@mkoeppe
Copy link
Member

mkoeppe commented Jun 29, 2022

comment:12

Thanks for making this change, LGTM

@mkoeppe
Copy link
Member

mkoeppe commented Jun 29, 2022

comment:13

(but I haven't tested it, so let's wait for the patchbot or the Build&test workflow)

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member

mkoeppe commented Jul 3, 2022

comment:15

Tested locally, seems to work fine. I was able to provoke errors in the testsuite by setting a low memory limit

@mkoeppe
Copy link
Member

mkoeppe commented Jul 3, 2022

Reviewer: Matthias Koeppe

@vbraun
Copy link
Member

vbraun commented Jul 9, 2022

Changed branch from u/dimpase/interfaces/gap_allow_memory_settings to e6ef4fe

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

4 participants