Fix gap_root doctest

[gh-timokau]

The recently introduced (#26665) doctest for gap_root (indended to test the fallback behaviour) breaks when GAP_ROOT_DIR gets reset as part of the sage initialization. We can just extract the fallback behaviour into a separate function instead which can be tested in isolation.

[git]

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

​06b7dae

Fix gap_root doctest when GAP_ROOT_DIR is reset

[fbissey]

Sure, why not. I didn't remember that some of my distro friend just use GAP_ROOT_DIR. I just consider it irrelevant and I hope gap-4.10 will make all that stuff just go away. I'll basically patch sage-on-gentoo so _guess_gap_root becomes the new gap_root and remove the warning.

But really that stuff was always stinky and if you want to know that variable you should just ask gap itself. My opinion.

[gh-timokau]

I agree, it would be nice if that wouldn't be necessary at all. Thanks for the quick review.

[embray]

Please don't use inline imports like import os.path in the gap_root function if it isn't necessary.

This might also conflict with some of the work I'm doing in #22626. Which is not to say I'd be opposed to fixing this issue first if it is an issue, but let me think for a sec about how that work relates to this.

[embray]

I think the code in gap_root should also check--just in case--that GAP_ROOT_DIR is of a type that's compatible with os.path.exists in the first place, or at least have a fallback if not. I realize the existing code didn't do that either, but as long as we're fixing things might as well make it more robust. Perhaps:

try:
    if os.path.isdir(GAP_ROOT_DIR):
        return GAP_ROOT_DIR
except TypeError:
    pass

return _guess_gap_root()

That would provide a reasonable fallback at least if GAP_ROOT_DIR is not actually a directory or a path-like type. Beyond that the onus is on the user for GAP_ROOT_DIR to actually be a sensible GAP root :)

[embray]

In the except TypeError it might also be worth outputting a warning with the warnings module since the user should know if they're giving a bogus GAP_ROOT_DIR. But since we have a reasonable fallback it shouldn't necessarily be an error either.

[gh-timokau]

Replying to embray:

In the except TypeError it might also be worth outputting a warning with the warnings module since the user should know if they're giving a bogus GAP_ROOT_DIR. But since we have a reasonable fallback it shouldn't necessarily be an error either.

Shouldn't it? If I explicitly pass in a GAP_ROOT_DIR, I would prefer it to fail loudly if that value is nonsense. So in that case the thrown TypeError may actually be fine, although I'm not opposed to add a nicer error message if you prefer.

Note that the inline import was also already there in the previous implementation and I just copied that. I never know what is supposed to be better style in python. From a purely style perspective I prefer to keep the import local, but maybe its not properly optimized?

[fbissey]

As I already mentioned, as far as I am concerned in sage-on-gentoo anything with GAP_ROOT_DIR is just removed. It has never been that useful as far as I am concerned. I hope that #22626 will provide us with an easy way to just ditch it.

May be I should just put my act together and provide an alternative to the whole thing that just interrogate the configured gap for gap_root. That would be more robust and potentially allow the use of an external gap more easily.

[gh-timokau]

Even if GAP_ROOT_DIR wouldn't be needed anymore, I'd still need a way to tell sage which gap binary to use. Currently the path is hardcoded. Ideally it would just use whatever is in PATH, but I guess sage wants to prefer its own SPKG.

[embray]

Replying to gh-timokau:

Replying to embray:

In the except TypeError it might also be worth outputting a warning with the warnings module since the user should know if they're giving a bogus GAP_ROOT_DIR. But since we have a reasonable fallback it shouldn't necessarily be an error either.

Shouldn't it? If I explicitly pass in a GAP_ROOT_DIR, I would prefer it to fail loudly if that value is nonsense. So in that case the thrown TypeError may actually be fine, although I'm not opposed to add a nicer error message if you prefer.

I'm a bit torn there: There is already a default value for GAP_ROOT_DIR that looks like a directory, so there's no great way to tell if it was set "explicitly". I wish there were.

Since it's your ticket I leave it up to you, though if nothing else I think the error should be caught and an exception with a more useful message raised. I might even make it a RuntimeError.

Note that the inline import was also already there in the previous implementation and I just copied that. I never know what is supposed to be better style in python. From a purely style perspective I prefer to keep the import local, but maybe its not properly optimized?

Ah, I missed that it was already there, but yes I would definitely clean that up.

In Python this almost always considered poor style: in part just as a semi-arbitrary stylistic choice, but also because adding an import statement always adds some overhead, and when it comes to core system modules it's especially pointless since they're almost always already imported; so the import statement might as well go at module-level where it's only evaluated once.

There are only a couple exceptions to this:

1) When some modifiable global variable needs to be imported with from foo import BAR that should generally not go at module-level since the value of BAR might change in foo. For example this is why you always almost use sys.path and not from sys import path. I think just accessing the variable through getattr on the module is better, but sometimes you'll see in Sage code things like from sage.env import WHATEVER at function-level, and I think that's okay.

2) Inline imports are often used to work around cases of circular imports which can break at module import-time. I think the prevalence of this in Sage might be what has led to an overproliferation of this poor style in general throughout Sage, even where it's not needed. This is especially less needed now in Sage with the use of LazyImport, which I think is a better way to get around circular import problems.

[git]

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

​7a66190

Improve the error message on invalid GAP_ROOT_DIR

[gh-timokau]

How about this?

Replying to embray:

Replying to gh-timokau:

Replying to embray:

In the except TypeError it might also be worth outputting a warning with the warnings module since the user should know if they're giving a bogus GAP_ROOT_DIR. But since we have a reasonable fallback it shouldn't necessarily be an error either.

Shouldn't it? If I explicitly pass in a GAP_ROOT_DIR, I would prefer it to fail loudly if that value is nonsense. So in that case the thrown TypeError may actually be fine, although I'm not opposed to add a nicer error message if you prefer.

I'm a bit torn there: There is already a default value for GAP_ROOT_DIR that looks like a directory, so there's no great way to tell if it was set "explicitly". I wish there were.

Well I think we can assume that the default value set by sage itself is at least a valid path. If it is not, it would be especially appropriate to fail loudly to make sure that never makes it into a release.

[embray]

True. The only way that could go wrong is if the Sage installation itself were broken. Theoretically a user can set a bogus $SAGE_LOCAL, but in that case we would also want to die much earlier. Right now I don't think such a case is well handled but the place for that would be sage.env.

[embray]

Fine for me. I would break up the one really long line, but I'm not going to fuss over it.

[embray]

I don't know why I added myself to the authors list. It wasn't intentional.

[gh-timokau]

Turns out I didn't really improve the situation for myself here. Now _guess_gap_root is always tested as it should. But the implementation assumes that gap is located in SAGE_LOCAL, which is not the case (which is why that guessing routine isn't used in the first place).

I guess I'm just going to patch it for now.

[embray]

I guess _guess_gap_root()'s tests should not be run in the case where _guess_gap_root() would not be used in the first place (this is one example of why I have a problem with writing doctests for every minor internal utility function).

I do want to do more work later on making Sage work well with a system GAP, which will be that much better with GAP 4.10 installs that provide libgap. Then we'll revisit this.

[gh-timokau]

Replying to embray:

I guess _guess_gap_root()'s tests should not be run in the case where _guess_gap_root() would not be used in the first place (this is one example of why I have a problem with writing doctests for every minor internal utility function).

I agree.

I do want to do more work later on making Sage work well with a system GAP, which will be that much better with GAP 4.10 installs that provide libgap.

That is now though isn't it?

Then we'll revisit this.

Thank you for making system dependencies a priority.