Opened 12 years ago

Closed 9 years ago

#378 closed enhancement (fixed)

User-specified path for load and attach

Reported by: was Owned by: was
Priority: major Milestone: sage-4.6.1
Component: user interface Keywords:
Cc: rossk Merged in: sage-4.6.1.alpha1
Authors: Mitesh Patel, Felix Lawrence Reviewers: Ross Kyprianou
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by mpatel)

The patch allows

sage: load_attach_path()
['.']
sage: load_attach_path('/path/to/my/sage/scripts')
sage: load_attach_path()
['.', '/path/to/my/sage/scripts']
sage: attach('nifty_script1.sage')
sage: attached_files()
['/path/to/my/sage/scripts/nifty_script1.sage']

You can also set an environment variable:

$ export SAGE_LOAD_ATTACH_PATH="$HOME/foo:$HOME/bar"
$ sage
sage: load_attach_path()
['.', '/home/mpatel/foo', '/home/mpatel/bar']

Note: We now use the full path in the attached files dictionary.

Attachments (4)

trac_378-load_attach_path.patch (8.4 KB) - added by mpatel 9 years ago.
First take on load / attach path. sage repo.
trac_378-load_attach_path.2.patch (12.3 KB) - added by mpatel 9 years ago.
More examples. Handle absolute paths. Replaces previous.
trac_378-load_attach_path.2.1.patch (9.0 KB) - added by flawrence 9 years ago.
use os.path.userexpand on load, separate reset function, also use paths for detach. replaces previous.
trac_387-load_attach_path.2.1.patch (14.0 KB) - added by flawrence 9 years ago.
use os.path.userexpand on load, separate reset function, also use paths for detach. actually replaces previous this time..

Download all attachments as: .zip

Change History (37)

comment:1 Changed 12 years ago by mabshoff

  • Milestone set to Sage-2.10

comment:2 Changed 9 years ago by mpatel

  • Report Upstream set to N/A

#1484 is related.

comment:3 Changed 9 years ago by mpatel

So is #516.

comment:4 Changed 9 years ago by mpatel

That should be #5169.

Changed 9 years ago by mpatel

First take on load / attach path. sage repo.

comment:5 Changed 9 years ago by mpatel

  • Authors set to Mitesh Patel
  • Description modified (diff)
  • Priority changed from minor to major
  • Status changed from new to needs_review
  • Summary changed from user-specified sage load path to User-specified path for load and attach

Feel free to improve the patch!

comment:6 Changed 9 years ago by mpatel

  • Description modified (diff)

comment:7 Changed 9 years ago by mpatel

  • Status changed from needs_review to needs_work

We should skip the search, if the given filename is an absolute path.

comment:8 Changed 9 years ago by mpatel

Should we add an option (recurse=False?) that makes load and attach search the entire directory tree under each search path?

Changed 9 years ago by mpatel

More examples. Handle absolute paths. Replaces previous.

comment:9 Changed 9 years ago by mpatel

  • Description modified (diff)
  • Status changed from needs_work to needs_review

With V2, we skip the search if given an absolute path. I've also added some examples.

comment:10 follow-ups: Changed 9 years ago by timdumol

Things work perfectly, but I think it might be better to follow the Python stdlib in exposing the array itself, instead of covering it in a function (sys.path instead of sys.path(), so similarly, load_attach_path instead of load_attach_path()). Also, it might be handy to os.path.expand_user any paths handed to the function, so you could have load_attach_path.append("~/foo"). What do you think?

comment:11 Changed 9 years ago by AlexGhitza

  • Status changed from needs_review to needs_info

comment:12 in reply to: ↑ 10 Changed 9 years ago by mpatel

  • Status changed from needs_info to needs_work

Replying to timdumol:

Things work perfectly, but I think it might be better to follow the Python stdlib in exposing the array itself, instead of covering it in a function (sys.path instead of sys.path(), so similarly, load_attach_path instead of load_attach_path()). Also, it might be handy to os.path.expand_user any paths handed to the function, so you could have load_attach_path.append("~/foo"). What do you think?

Both ideas sound great. I can't make the changes immediately, but I'll try to attach a new patch soon.

comment:13 Changed 9 years ago by rossk

  • Cc rossk added

Available as another reviewer if you need one.

comment:14 Changed 9 years ago by jsrn

Is this still alive? I found myself in need of this functionality today and was surprised it is not there; how does anyone use the notebook locally without it?

Anyway, I'd like to help out fixing or reviewing if necessary. I've only glanced at the current patch but found this anomaly: Shouldn't line 1399 be changed to

while '' in _load_attach_path:

so as to remove any number of empty entries (e.g. if user wrote SAGE_LOAD_ATTACH_PATH = ":::::foo::")

Cheers, Johan

comment:15 Changed 9 years ago by mpatel

Thanks for the interest! Unfortunately, I've been busy with other tasks (Sage and non-Sage) and haven't had the time to rework the patch to incorporate Tim's suggestions. Is anyone willing to do that?

comment:16 in reply to: ↑ 10 ; follow-up: Changed 9 years ago by flawrence

Replying to timdumol:

Things work perfectly, but I think it might be better to follow the Python stdlib in exposing the array itself, instead of covering it in a function (sys.path instead of sys.path(), so similarly, load_attach_path instead of load_attach_path()). Also, it might be handy to os.path.expand_user any paths handed to the function, so you could have load_attach_path.append("~/foo"). What do you think?

I've had a bit of a hack at exposing the array itself, but there are problems making it sufficiently global. sys.path works for the stdlib because path is a variable in the sys module. The logical thing to do here is to create a load_attach_path variable in the preparser module, but following the stdlib analogy this would require the user to edit sage.misc.preparser.load_attach_path, which is not very user friendly.

Failing this, it looks like the best you can do is a from sage.misc.preparser import load_attach_path, but this is a fragile state of affairs: if you reassign load_attach_path anywhere, either from sage's command line or by calling some function like sage.misc.preparser.reset_load_attach_path(), you end up with a situation where load_attach_path and sage.misc.preparser.load_attach_path are different objects.

This requires caution on the part of the user and the programmer. For example, reset_load_attach_path() can't include

load_attach_path = ['.']

instead it has to do

while load_attach_path != []:
    load_attach_path.pop()
load_attach_path.append('.')

If the user accidentally reassigns load_attach_path in the command line, they'll have to re-run from sage.misc.preparser import load_attach_path. I don't think that this step can be included in a reset_load_attach_path() function due to the different scopes.

So it's possible to expose the load_attach_path array to the user, and I've written most of a patch that does this, but there's lots of things that could go wrong so I'd suggest sticking to the existing patch's approach of hiding behind a function. If others think that the pros outweigh the cons then I'll tidy up the patch and upload it.

Thoughts?

Changed 9 years ago by flawrence

use os.path.userexpand on load, separate reset function, also use paths for detach. replaces previous.

comment:17 Changed 9 years ago by flawrence

  • Status changed from needs_work to needs_review

Assuming that no-one comes up with a robust and convenient way to expose the array, I've uploaded a patch that addresses Tim's other idea, Johan's comment, and two other main changes:

  • You can now use paths with detach
  • reset_load_attach_path is now a function on its own, which gets called upon startup. Note that it now resets the path to '.' plus the contents of the path environment variable SAGE_LOAD_ATTACH_PATH, rather than to '.' alone.

comment:18 Changed 9 years ago by rossk

Reviewing this on 4.6.1.alpha1 - should I be using 4.6?

/scratch/rossk/sage-4.6.1/devel/sage$ hg qpush
applying trac_378-load_attach_path.2.1.patch
patching file sage/misc/all.py
Hunk #1 FAILED at 57
1 out of 1 hunks FAILED -- saving rejects to file sage/misc/all.py.rej
patching file sage/misc/preparser.py
Hunk #1 FAILED at 1390
Hunk #2 FAILED at 1488
Hunk #3 FAILED at 1497
Hunk #4 FAILED at 1533
Hunk #5 FAILED at 1568
Hunk #6 FAILED at 1582
Hunk #7 FAILED at 1593
Hunk #8 FAILED at 1608
Hunk #9 FAILED at 1618
Hunk #10 FAILED at 1771
10 out of 10 hunks FAILED -- saving rejects to file sage/misc/preparser.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh trac_378-load_attach_path.2.1.patch

Changed 9 years ago by flawrence

use os.path.userexpand on load, separate reset function, also use paths for detach. actually replaces previous this time..

comment:19 follow-up: Changed 9 years ago by flawrence

Sorry, I'm new to mercurial queues and stuffed up the patch (it did rely on the other patch after all). The new patch I just uploaded should work on 4.6 without mpatel's patch, and hopefully 4.6.alpha1 too.

comment:20 in reply to: ↑ 19 Changed 9 years ago by rossk

Replying to flawrence:

Sorry, I'm new to mercurial queues and stuffed up the patch (it did rely on the other patch after all). The new patch I just uploaded should work on 4.6 without mpatel's patch, and hopefully 4.6.alpha1 too.

Im new to mercurial queues too (and much more :-). Thought youd like to know your new patch was error free. (Ill test over the weekend and report back when I can - sorry it cant be sooner).

comment:21 in reply to: ↑ 16 ; follow-up: Changed 9 years ago by jsrn

Replying to flawrence:

Replying to timdumol:

Things work perfectly, but I think it might be better to follow the Python stdlib in exposing the array itself, instead of covering it in a function (sys.path instead of sys.path(), so similarly, load_attach_path instead of load_attach_path()). Also, it might be handy to os.path.expand_user any paths handed to the function, so you could have load_attach_path.append("~/foo"). What do you think?

I've had a bit of a hack at exposing the array itself, but there are problems making it sufficiently global. sys.path works for the stdlib because path is a variable in the sys module. The logical thing to do here is to create a load_attach_path variable in the preparser module, but following the stdlib analogy this would require the user to edit sage.misc.preparser.load_attach_path, which is not very user friendly.

Failing this, it looks like the best you can do is a from sage.misc.preparser import load_attach_path, but this is a fragile state of affairs: if you reassign load_attach_path anywhere, either from sage's command line or by calling some function like sage.misc.preparser.reset_load_attach_path(), you end up with a situation where load_attach_path and sage.misc.preparser.load_attach_path are different objects.

This requires caution on the part of the user and the programmer. For example, reset_load_attach_path() can't include

load_attach_path = ['.']

instead it has to do

while load_attach_path != []:
    load_attach_path.pop()
load_attach_path.append('.')

If the user accidentally reassigns load_attach_path in the command line, they'll have to re-run from sage.misc.preparser import load_attach_path. I don't think that this step can be included in a reset_load_attach_path() function due to the different scopes.

So it's possible to expose the load_attach_path array to the user, and I've written most of a patch that does this, but there's lots of things that could go wrong so I'd suggest sticking to the existing patch's approach of hiding behind a function. If others think that the pros outweigh the cons then I'll tidy up the patch and upload it.

Thoughts?

Very astute! I was almost finished with my own patch, falling into all the same holes as you describe :-) Your concern at least begs the question on whether this is what we want to do, as it might fool some users. However, a possible usage pattern - in case load_attach_path is fully exposed - is the following

   import sage.misc.preparser as pre
   pre.load_attach_path = [ ... ]

Still, this is not very nice, and doesn't really fit into the "import sage.all" scheme. A third possibility is to "decide" that we might very well need such global variables in other places, stemming from various other foundational modules, and therefore introduce a new module sage.misc.globals to hold these. When Sage launches, it imports this module as "globals", and so, for any global variable - say load_attach_path -, the user would access it and modify it as globals.load_attach_path. I don't think that would be unnatural at all. However, it might be overkill if load_attach_path is the _only_ such conceivable global value, but for some reason (perhaps the sheer size of Sage), I can't really think it is.

If none of these sound compelling, then I vote yes on the current form (I haven't tested the patch, that is). My only comment on the current patch, is that maybe there should be a TODO (or a fix ;-) ) in reset_load_attach_path(), which notes that probably the elements of SAGE_LOAD_ATTACH_PATH should be uniquified while preserving the order (thus, list(set(_load_attach_path)) is no good). After that the while could be reverted to an if again when removing the empty element.

Otherwise good work. I'm looking forward to this patch in Sage, whatever the format :-) Cheers, Johan

comment:22 Changed 9 years ago by mpatel

Thanks to everyone for working on this ticket!

One question that may relate to how/whether we "expose" the path list: Would users want Sage to search some paths recursively for files to load or attach? How would we store this information? Perhaps as

['/some/path', ('/a/path/to/search/recursively', True), '/another/path']

? Of course, this needn't hold up this ticket.

comment:23 in reply to: ↑ 21 Changed 9 years ago by flawrence

  • Authors changed from Mitesh Patel to Mitesh Patel, Felix Lawrence

Replying to jsrn:

My only comment on the current patch, is that maybe there should be a TODO (or a fix ;-) ) in reset_load_attach_path(), which notes that probably the elements of SAGE_LOAD_ATTACH_PATH should be uniquified while preserving the order (thus, list(set(_load_attach_path)) is no good). After that the while could be reverted to an if again when removing the empty element.

Do we really need to test for uniqueness at all? It's the user's fault if they get it wrong, and the only negative consequence I see is that attaching/loading files (not the sort of operation that sits deep in a loop) a tiny tiny bit slower. Python doesn't check for uniqueness in sys.path, BASH doesn't check for uniqueness in $PATH. AFAIK the main argument for uniqueness is aesthetics; perhaps we can leave this to the user rather than try to tidy up after them (and perhaps do something unexpected in the process)?

Replying to mpatel:

One question that may relate to how/whether we "expose" the path list: Would users want Sage to search some paths recursively for files to load or attach? How would we store this information?

This is an interesting question. I think it would be a nice feature. Since we don't actually expose the path array, it would be easy to add such features later, so it's probably best as a new ticket.

I also noticed that I was fat-fingered typing the name of the new patch - 387 instead of 378. *sigh*

comment:24 Changed 9 years ago by rossk

Quick question: attached_files() works without an import but load_attach_path() is unknown (raises a "NameError?" exception when called). Are we supposed to import it in order to use it?

comment:25 follow-up: Changed 9 years ago by flawrence

You shouldn't have to import load_attach_path() manually. Applying the patch and rebuilding on 4.6 worked for me. Maybe you forgot to rebuild sage after applying the most recent patch?

comment:26 in reply to: ↑ 25 Changed 9 years ago by rossk

Replying to flawrence:

You shouldn't have to import load_attach_path() manually. Applying the patch and rebuilding on 4.6 worked for me. Maybe you forgot to rebuild sage after applying the most recent patch?

Doh! Certainly did forget and rebuilding worked - thanks!

comment:27 follow-up: Changed 9 years ago by rossk

I exercised this reasonably well and the functionality is great. After it passed all the tests I could think of, I renamed the folder that had the attached file (to see what would happen). The error was not unexpected: "OSError: [Errno 2] No such file or directory: ...". But this error persisted no matter what was done afterwards (even just "1+1"!). I had to get out of Sage to start again. Thats the most pressing issue I could find but otherwise looks good! Theres also a couple of ideas I can run by you if you want, now that Ive exercised all the use cases I could think of.

comment:28 in reply to: ↑ 27 Changed 9 years ago by flawrence

Replying to rossk:

I renamed the folder that had the attached file (to see what would happen). The error was not unexpected: "OSError: [Errno 2] No such file or directory: ...". But this error persisted no matter what was done afterwards (even just "1+1"!). I had to get out of Sage to start again.

I've checked and this happens even without this patch applied, so it's a separate issue I think. Not much error checking is done for attached files! I've started ticket #10229 for this.

comment:29 Changed 9 years ago by rossk

Because the functionality is important, it works as designed, and passed all tests (the path can be set, displayed and modified as intended), this ticket can get a positive review now. If nobody objects, Ill do that after a short wait for any feedback on the next comment.

comment:30 Changed 9 years ago by rossk

It may be too late for the following suggestions but Ill put them forward in case you think they have enough merit to refactor the code now (otherwise ignore them or they can go in another ticket). They are based on the idea that the main things we want to do with a path is: (1) start Sage with a sensible default (2) display what the current path is (3) set the path (4) add a folder to the end/beginning of the path (5) reset the path (6) totally clear the path. (I acknowledge some ideas have come from other packages e.g. matlab). So here's is how the path may be made to work in the future. Please let me know what you think. These are just ideas and doesnt change the good work youve already done - thanks for that!

# display the default attach-path (unless a SAGE_ATTACH_PATH environment variable is set, then the attach-path when Sage starts is the path in SAGE_ATTACH_PATH)
sage: attach_path()
['.']

# set the attach path to a list of folders: ['./folder1','./folder2']
# (Note: if you want the current directory '.' included, you must specify it)
sage: attach_path(['folder1','folder2'])

# append to the (end of) attach path
sage: attach_path_join(attach_path(), ['folder3'])

# display the attach path 
sage: attach_path()
['folder1','folder2','folder3']

# add to the start of the attach path
sage: attach_path_join(['folder0'], attach_path())

# display the attach path again
sage: attach_path()
['folder0','folder1','folder2','folder3']

# append to the (end of) attach path *recursively*
sage: attach_path_join_subfolders(attach_path(), ['folder4'])

# display the attach path 
sage: attach_path()
['folder0','folder1','folder2','folder3','folder4','folder4/dir1','folder4/dir2']

# reset the attach path - sets the attach path to the default 
sage: reset_attach_path()

# clear the attach path - sets it to '' i.e. neither '.' nor SAGE_ATTACH_PATH  are included (when you want to ensure no implicit attaching is done or to rebuild the path from scratch)
sage: clear_attach_path()

# display the attach path 
sage: attach_path()
[]

comment:31 Changed 9 years ago by rossk

  • Status changed from needs_review to positive_review

Time to give this the positive review it deserves. Looking forward to using attach paths!

comment:32 Changed 9 years ago by jdemeyer

  • Reviewers set to Ross Kyprianou

comment:33 Changed 9 years ago by jdemeyer

  • Merged in set to sage-4.6.1.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.