Opened 12 months ago

Closed 12 months ago

Last modified 10 months ago

#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) 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 12 months ago by gh-timokau

  • Branch changed from u/gh-timokau/fix-gap_root-doctest to u/gh-timokau/fix-gap_root-test
  • Commit set to f894105d0d9e172c6ec8b0ed2d6bf92e9b5b31e2
  • Status changed from new to needs_review

comment:2 Changed 12 months ago by git

  • Commit changed from f894105d0d9e172c6ec8b0ed2d6bf92e9b5b31e2 to 06b7daefa71636190b166d896bb0dd221a179cf7

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

06b7daeFix gap_root doctest when GAP_ROOT_DIR is reset

comment:3 Changed 12 months ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to 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 12 months ago by gh-timokau

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

comment:5 Changed 12 months ago by embray

  • Status changed from positive_review to 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.

Last edited 12 months ago by embray (previous) (diff)

comment:6 Changed 12 months ago by 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 :)

comment:7 follow-up: Changed 12 months ago by 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.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 12 months ago by 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?

comment:9 Changed 12 months ago by 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.

comment:10 Changed 12 months ago by 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.

comment:11 in reply to: ↑ 8 ; follow-up: Changed 12 months ago by 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.

comment:12 Changed 12 months ago by git

  • Commit changed from 06b7daefa71636190b166d896bb0dd221a179cf7 to 7a661907c1db86447516304ce8c45f99ce7d3510

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

7a66190Improve the error message on invalid GAP_ROOT_DIR

comment:13 in reply to: ↑ 11 Changed 12 months ago by 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.

comment:14 Changed 12 months ago by 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.

comment:15 Changed 12 months ago by embray

  • Authors changed from Timo Kaufmann to Timo Kaufmann, Erik Bray
  • Status changed from needs_work to 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 12 months ago by vbraun

  • Branch changed from u/gh-timokau/fix-gap_root-test to 7a661907c1db86447516304ce8c45f99ce7d3510
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:17 Changed 12 months ago by embray

  • Authors changed from Timo Kaufmann, Erik Bray to Timo Kaufmann
  • Commit 7a661907c1db86447516304ce8c45f99ce7d3510 deleted
  • Reviewers changed from François Bissey to François Bissey, Erik Bray

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

comment:18 Changed 12 months ago by 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.

comment:19 follow-up: Changed 10 months ago by 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.

comment:20 in reply to: ↑ 19 Changed 10 months ago by 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.

Note: See TracTickets for help on using tickets.