#26758 closed defect (fixed)
Fix gap_root doctest
Reported by: | gh-timokau | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.5 |
Component: | misc | Keywords: | gap |
Cc: | fbissey | Merged in: | |
Authors: | Timo Kaufmann | Reviewers: | François Bissey, Erik Bray |
Report Upstream: | N/A | Work issues: | |
Branch: | 7a66190 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description
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.
Change History (20)
comment:1 Changed 4 years ago by
Branch: | u/gh-timokau/fix-gap_root-doctest → u/gh-timokau/fix-gap_root-test |
---|---|
Commit: | → f894105d0d9e172c6ec8b0ed2d6bf92e9b5b31e2 |
Status: | new → needs_review |
comment:2 Changed 4 years ago by
Commit: | f894105d0d9e172c6ec8b0ed2d6bf92e9b5b31e2 → 06b7daefa71636190b166d896bb0dd221a179cf7 |
---|
comment:3 Changed 4 years ago by
Reviewers: | → François Bissey |
---|---|
Status: | needs_review → positive_review |
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.
comment:4 Changed 4 years ago by
I agree, it would be nice if that wouldn't be necessary at all. Thanks for the quick review.
comment:5 Changed 4 years ago by
Status: | positive_review → needs_work |
---|
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.
comment:6 Changed 4 years ago by
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 :)
comment:7 follow-up: 8 Changed 4 years ago by
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.
comment:8 follow-up: 11 Changed 4 years ago by
Replying to embray:
In the
except TypeError
it might also be worth outputting a warning with thewarnings
module since the user should know if they're giving a bogusGAP_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?
comment:9 Changed 4 years ago by
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.
comment:10 Changed 4 years ago by
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.
comment:11 follow-up: 13 Changed 4 years ago by
Replying to gh-timokau:
Replying to embray:
In the
except TypeError
it might also be worth outputting a warning with thewarnings
module since the user should know if they're giving a bogusGAP_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 thrownTypeError
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.
comment:12 Changed 4 years ago by
Commit: | 06b7daefa71636190b166d896bb0dd221a179cf7 → 7a661907c1db86447516304ce8c45f99ce7d3510 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
7a66190 | Improve the error message on invalid GAP_ROOT_DIR
|
comment:13 Changed 4 years ago by
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 thewarnings
module since the user should know if they're giving a bogusGAP_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 thrownTypeError
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.
comment:14 Changed 4 years ago by
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
.
comment:15 Changed 4 years ago by
Authors: | Timo Kaufmann → Timo Kaufmann, Erik Bray |
---|---|
Status: | needs_work → positive_review |
Fine for me. I would break up the one really long line, but I'm not going to fuss over it.
comment:16 Changed 4 years ago by
Branch: | u/gh-timokau/fix-gap_root-test → 7a661907c1db86447516304ce8c45f99ce7d3510 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:17 Changed 4 years ago by
Authors: | Timo Kaufmann, Erik Bray → Timo Kaufmann |
---|---|
Commit: | 7a661907c1db86447516304ce8c45f99ce7d3510 |
Reviewers: | François Bissey → François Bissey, Erik Bray |
I don't know why I added myself to the authors list. It wasn't intentional.
comment:18 Changed 4 years ago by
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.
comment:19 follow-up: 20 Changed 4 years ago by
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.
comment:20 Changed 4 years ago by
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.
Branch pushed to git repo; I updated commit sha1. New commits:
Fix gap_root doctest when GAP_ROOT_DIR is reset