Opened 12 years ago
Last modified 11 years ago
#9739 closed defect
Handle duplicate file basenames when testing multiple files in parallel — at Version 75
Reported by: | mpatel | Owned by: | mvngu |
---|---|---|---|
Priority: | blocker | Milestone: | sage-4.7.2 |
Component: | doctest coverage | Keywords: | doctest scripts race condition unique filenames ptestlong -tp |
Cc: | drkirkby, jhpalmieri, leif, robertwb, jdemeyer | Merged in: | |
Authors: | Mitesh Patel, John Palmieri | Reviewers: | Robert Bradshaw, Leif Leonhardy |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
When we test
/path/to/foo.py
,
sage-doctest
writes
SAGE_TESTDIR/.doctest_foo.py
,
runs the new file through python
, and deletes it. This can cause collisions when we test in parallel multiple files with the same basename, e.g., __init__
, all
, misc
, conf
, constructor
, morphism
, index
, tests
, homset
, element
, twist
, tutorial
, sagetex
, crystals
, cartesian_product
, template
, ring
, etc.
There's a similar problem with testing non-library files, which sage-doctest
first effectively copies to SAGE_TESTDIR
.
See sage-devel for background.
This ticket may help with some of the doctesting problems discussed on the Sage mailing lists. Related tickets: #9224, #9225, #9449.
There's now a meta-ticket for all doctest-related issues / tickets, #11337.
I don't know much about this, so don't feel able to work on it.
But for those that do, perhaps a sub-optimal solution is a better temporary measure than a complete industrial strength bullet-proof solution.
Normally I don't like "quick fixes" that don't do the job properly, but in this case it seems if something is not done, then the problem will remain open forever, as nothing has actually been done on it for 7 months.
Sometimes I feel that trac tickets get held up for unnecessary long periods due to a reviewer being over pedantic and wanting things outside the scope of the original ticket, which would be better put on another ticket.
(Dave)
Apply
- trac_9739.v2.patch to the scripts repository
- trac_9739-graphviz.patch to the Sage library
Change History (81)
comment:1 Changed 12 years ago by
- Description modified (diff)
Changed 12 years ago by
Frequency-sorted list of doctest file basenames (includes .py, .pyx, .pxi, .rst, .tex files). Not a patch.
comment:2 Changed 12 years ago by
I've attached a list of ~1500 unique basenames for the ~2500 files we doctest (give or take a handful).
comment:3 Changed 12 years ago by
- Cc leif added
comment:4 Changed 12 years ago by
- Cc drkirkby jhpalmieri added
- Milestone set to sage-4.6
- Status changed from new to needs_review
I've attached a patch. I haven't modified sage-test
, since we're likely to phase it out at #9224.
comment:5 follow-up: ↓ 7 Changed 12 years ago by
Hmmm, I'm not sure if people will like substituting the filename with the whole test command everywhere. (I have a wide screen, but...)
Can we please stop creating "hidden" doctest files? To me that doesn't make sense at all; especially but not limited to files in ~/.sage/*/
.
Does the "For whitespace errors, see ..." still work (i.e., print the correct filename)?
The notion of "absolute filenames relative to ..." is a bit weird... (But thanks for renaming abs()
. :) )
The rest seems ok; I've yet only looked at the patch though.
comment:6 Changed 12 years ago by
While printing the number of global and file iterations even if both are 1 isn't that useful, I'd like to see the current / appropriate timeout setting[s] printed in sage-ptest
(i.e. SAGE_TIMEOUT
for normal tests, and only SAGE_TIMEOUT_LONG
when -long
was given).
(In principle, files containing no examples marked # long time
should IMHO not be tested with SAGE_TIMEOUT_LONG
, but the normal timeout. In that case we should print both timeout settings, and perhaps also indicate with which a file is being tested.)
comment:7 in reply to: ↑ 5 Changed 12 years ago by
Replying to leif:
Hmmm, I'm not sure if people will like substituting the filename with the whole test command everywhere. (I have a wide screen, but...)
Ok, sorry, hard to read. The behavoir seems to be the same as before...
comment:8 follow-up: ↓ 9 Changed 12 years ago by
How should I best apply this:
drkirkby@hawk:~/2/sage-4.5.3.alpha2$ cd local/bin drkirkby@hawk:~/2/sage-4.5.3.alpha2/local/bin$ hg qimport http://trac.sagemath.org/sage_trac/raw-attachment/ticket/9739/trac_9739-unique_doctest_names.patch adding trac_9739-unique_doctest_names.patch to series file drkirkby@hawk:~/2/sage-4.5.3.alpha2/local/bin$ hg push abort: repository /space/rlm/sage-4.1.rc1/local/bin not found!
Something, somewhere seems to be looking for some directory of what I assume is Robert Millers.
Dave
comment:9 in reply to: ↑ 8 Changed 12 years ago by
Replying to drkirkby:
Something, somewhere seems to be looking for some directory of what I assume is Robert Millers.
What happens if you (re)move SAGE_LOCAL/bin/.hg/hgrc
? The problem might be that when you're in SAGE_LOCAL/bin
, you're invoking ./hg
, instead of /usr/local/bin/hg
.
comment:10 Changed 12 years ago by
- Cc robertwb added
comment:11 follow-up: ↓ 12 Changed 12 years ago by
Note that you don't want to create a race condition with two tests trying to create the same directory at the same time. Perhaps mangling "/" -> "." would be sufficient.
comment:12 in reply to: ↑ 11 Changed 12 years ago by
- Reviewers set to Robert Bradshaw
Replying to robertwb:
Note that you don't want to create a race condition with two tests trying to create the same directory at the same time. Perhaps mangling "/" -> "." would be sufficient.
Thanks, Robert. How about adding the process ID, too, or instead? I'd also like to reduce the chance of races when we run multiple sage -t(p)
commands simultaneously with the same DOT_SAGE
. Or are there other potential races under this directory, e.g., in DOT_SAGE/gap
?
comment:13 Changed 12 years ago by
- Status changed from needs_review to needs_work
comment:14 Changed 12 years ago by
Has there been any more thoughts on this?
I had a doctest failure today on my OpenSolaris machine hawk, which is almost certainly a result of this bug. This is using sage-4.6.rc0.
The following tests failed: sage -t -long local/lib/python2.6/site-packages/sagenb-0.8.2-py2.6.egg/sagenb/notebook/misc.py # 0 doctests failed sage -t -long devel/sage/sage/plot/plot.py # 5 doctests failed
The likely reason for the first of these failures can be seen if we look further up the log.
sage -t -long local/lib/python2.6/site-packages/sagenb-0.8.2-py2.6.egg/sagenb/misc/misc.py [2.9 s] sage -t -long local/lib/python2.6/site-packages/sagenb-0.8.2-py2.6.egg/sagenb/notebook/misc.py python: can't open file '/export/home/drkirkby/.sage//tmp/misc.py': [Errno 2] No such file or directory [0.2 s]
The log clearly shows a file misc.py
being tested, followed by a second test with a different file also called misc.py
. That file is then not found. It seems almost inevitable one test deleted the file needed by another test as they were both called misc.py
and both tested around the same time.
Note this is the same machine on which the buildbot passed all doctests.
http://build.sagemath.org/sage/builders/hawk%20full/builds/8/steps/shell_6/logs/stdio
so it seems to be an intermittent problem. (I've also had all tests pass on this).
These doctests issues are really annoying, as one never knows if its a real Sage bug, or a doctest bug.
Dave
comment:15 Changed 12 years ago by
- Cc jdemeyer added
comment:16 follow-up: ↓ 17 Changed 12 years ago by
I'm planning to return to this soon, probably after 4.6 is out.
comment:17 in reply to: ↑ 16 Changed 12 years ago by
Replying to mpatel:
I'm planning to return to this soon, probably after 4.6 is out.
That would be great, because I have also hit this error quite a few times.
comment:18 Changed 12 years ago by
- Keywords doctest scripts added
- Milestone changed from sage-4.6 to sage-4.6.1
- Priority changed from critical to blocker
comment:19 follow-up: ↓ 20 Changed 12 years ago by
There is another problem, which could exist even if every file had a different name.
If one tests multiple instances of Sage serially, then since they both write to $HOME/.sage, failures can occur even if the file names of the doctests are unique to any one copy of Sage. They need to be unique for any number of instances of Sage. I think testing under $HOME/.sage is a bit silly myself - it would be better to test under the directory where Sage is installed.
I found that testing devel/sage/sage/libs/fplll/fplll.pyx
(see #10195), would generate problems when I was testing two copies of Sage at the same time (slightly different versions). I don't know if this patch can handle that situation, but it would be good if it could.
Dave
comment:20 in reply to: ↑ 19 ; follow-up: ↓ 21 Changed 12 years ago by
Replying to drkirkby:
There is another problem, which could exist even if every file had a different name.
If one tests multiple instances of Sage serially, then since they both write to $HOME/.sage, failures can occur even if the file names of the doctests are unique to any one copy of Sage.
Well this would definitely be a user error. You can always set DOT_SAGE
or SAGE_TESTDIR
(or whatever it is called) if you want to run multiple tests simultaneously in different shells, even in / with the same Sage installation.
They need to be unique for any number of instances of Sage. I think testing under $HOME/.sage is a bit silly myself - it would be better to test under the directory where Sage is installed.
Definitely not, since this wouldn't work for site installations, where users usually have no write permissions under SAGE_ROOT
.
I don't know if this patch can handle that situation, but it would be good if it could.
One could use Sage's PID, user and machine parameters etc. to try to create unique directories, or generally create "random" directories with mktemp (1)
or mkdtemp()
, but I think this would be an overkill, since the user can itself do such by setting one of the above variables.
comment:21 in reply to: ↑ 20 ; follow-up: ↓ 22 Changed 12 years ago by
Replying to leif:
Replying to drkirkby:
There is another problem, which could exist even if every file had a different name.
If one tests multiple instances of Sage serially, then since they both write to $HOME/.sage, failures can occur even if the file names of the doctests are unique to any one copy of Sage.
Well this would definitely be a user error. You can always set
DOT_SAGE
orSAGE_TESTDIR
(or whatever it is called) if you want to run multiple tests simultaneously in different shells, even in / with the same Sage installation.They need to be unique for any number of instances of Sage. I think testing under $HOME/.sage is a bit silly myself - it would be better to test under the directory where Sage is installed.
Definitely not, since this wouldn't work for site installations, where users usually have no write permissions under
SAGE_ROOT
.I don't know if this patch can handle that situation, but it would be good if it could.
One could use Sage's PID, user and machine parameters etc. to try to create unique directories, or generally create "random" directories with
mktemp (1)
ormkdtemp()
, but I think this would be an overkill, since the user can itself do such by setting one of the above variables.
I disagree. I don't think creating unique temporary files is overkill. It would be far less error prone. With a test you want to run many times, having a dozen copies of Sage around for test purposes is quite a sensible thing to do with multi-core machines. Havving to set DOT_SAGE for each is annoying, when a unique temporary file could be made.
Dave
comment:22 in reply to: ↑ 21 Changed 12 years ago by
Replying to drkirkby:
I disagree. I don't think creating unique temporary files is overkill.
I didn't say that, but it IMHO suffices to have unique names in the namespace of a ptest run.
It would be far less error prone. With a test you want to run many times, having a dozen copies of Sage around for test purposes is quite a sensible thing to do with multi-core machines. Havving to set DOT_SAGE for each is annoying, when a unique temporary file could be made.
As I said, you can automatically set up unique test directories by setting one of the above variables e.g. based on the "login" shell's PID, one per terminal / shell.
If multiple machines share the same filesystem, simply add e.g. the hostname, too.
comment:23 follow-up: ↓ 24 Changed 12 years ago by
+1 to temp (per instance) directories. They would get cleaned up properly, and /tmp is almost always fast and local which is another plus.
(Really, we shouldn't have to be writing these files out at all...)
comment:24 in reply to: ↑ 23 ; follow-up: ↓ 26 Changed 12 years ago by
Replying to robertwb:
+1 to temp (per instance) directories. They would get cleaned up properly, and /tmp is almost always fast and local which is another plus.
(Really, we shouldn't have to be writing these files out at all...)
I'm not sure if you are agreeing with me or Leif there Robert - perhaps you can clarify.
I was going to suggest that we should be using /tmp, but I did not since I can see a disadvantage of it. NFS file systems have caused problems with doc tests failing, especially if they are mis-configured. As such, it would be better if a user tested Sage on the file system where it will be used. By using /tmp they might get a false sense of security. That said, using /tmp for temporary files has been the norm for years.
I personally think where reasonably practical, we should stop multiple instances of running Sage tests interfering with each other. Although Leif considers this a user error, it is one that a user might easily make.
But if it possible to avoid creating temporary files, then that should be done. But I would imagine that requires more changes than just adding a pid or hostname.
Dave
comment:25 follow-up: ↓ 27 Changed 12 years ago by
OT: My make ptestlong
of Sage 4.6 on Ubuntu 9.04 x86 somehow went into an infinite loop...
This never happened before (in dozens of builds), any idea?
(I started the complete build with make ptestlong
, build succeeded and building the documentation went normal, but now I'm meanwhile at the tenth doctest run! Checked this with grep Doctesting ptestlong.log
.)
comment:26 in reply to: ↑ 24 Changed 12 years ago by
Replying to drkirkby:
Replying to robertwb:
+1 to temp (per instance) directories. They would get cleaned up properly, and /tmp is almost always fast and local which is another plus.
(Really, we shouldn't have to be writing these files out at all...)
I'm not sure if you are agreeing with me or Leif there Robert - perhaps you can clarify.
Agreeing with drkirkby, we should use temp directories.
I was going to suggest that we should be using /tmp, but I did not since I can see a disadvantage of it. NFS file systems have caused problems with doc tests failing, especially if they are mis-configured. As such, it would be better if a user tested Sage on the file system where it will be used. By using /tmp they might get a false sense of security. That said, using /tmp for temporary files has been the norm for years.
Don't know if mktemp is POSIX, but it's widely available. Of course from Python you always have http://docs.python.org/library/tempfile.html Both are much better than manually specifying /tmp.
I personally think where reasonably practical, we should stop multiple instances of running Sage tests interfering with each other. Although Leif considers this a user error, it is one that a user might easily make.
I don't consider it a user error, and I also don't like filling .sage with lots of junk.
But if it possible to avoid creating temporary files, then that should be done. But I would imagine that requires more changes than just adding a pid or hostname.
True, and this ticket has been opened for far too long.
- Robert
comment:27 in reply to: ↑ 25 Changed 12 years ago by
Replying to leif:
OT: My
make ptestlong
of Sage 4.6 on Ubuntu 9.04 x86 somehow went into an infinite loop...This never happened before (in dozens of builds), any idea?
(I started the complete build with
make ptestlong
, build succeeded and building the documentation went normal, but now I'm meanwhile at the tenth doctest run! Checked this withgrep Doctesting ptestlong.log
.)
Ouch. I just noticed I had set SAGE_TEST_GLOBAL_ITER=1000
earlier in that shell...
(But all tests passed; I then aborted it during the 14th run.)
comment:28 follow-up: ↓ 29 Changed 12 years ago by
What is the current status of this patch, do you consider it ready for review? If not, I'm willing to join in and help where needed.
comment:29 in reply to: ↑ 28 Changed 12 years ago by
Replying to jdemeyer:
What is the current status of this patch, do you consider it ready for review? If not, I'm willing to join in and help where needed.
It's not ready for review. I don't think I have the time to work on this in the next several days, so if you'd like to help, please do!
I think we can make a temporary directory for each run in sage-(p)test
and pass it to sage-doctest
, which should ensure the temporary files under the directory are unique. The existing doesn't quite yet do the latter, but it should be easy to modify it so it does (e.g., with full paths and/or pids, etc.).
comment:30 Changed 12 years ago by
Using both a hostname and a PID should mean the filename is practically unique if people test on more than one computer using a shared drive. I don't think 'mktemp' will be unique on NFS shared drives, though the probability of a collision would then be very small indeed. But adding a hostname would reduce it even further.
But we need to be careful if using mktemp. Whilst many systems have it, the implementation is not the same on every system. I know Solaris works a bit different to Linux or OS X (I forget which). I know using something on Solaris with mktemp which would not work with Linux or OS X. (I forget which OS it was though). It seems HP-UX and Solaris differ too.
I would be very keen to use something that will work on AIX. There is a chance of IBM donating a quad core 4.5 GHz machine to the Sage project with AIX on it.
Anyway, whatever solution is used, I think it will be 1000x better than the current solution, but personally I'd like to see something that's unique to a machine and portable.
Dave
comment:31 Changed 11 years ago by
- Milestone changed from sage-4.6.1 to sage-4.6.2
I'm still interested in working on this patch, but I don't expect it to be ready on time for 4.6.1.
comment:32 Changed 11 years ago by
While we are at it, I have some more comments on sage-ptest
I'll also post here with an inline patch:
-
sage-ptest
diff --git a/sage-ptest b/sage-ptest
a b 81 81 Returns true if the file should not be tested 82 82 """ 83 83 if not os.path.exists(F): 84 # XXX IMHO this should never happen; in case it does, it's certainly 85 # an error to be reported (either filesystem, or bad name specified 86 # on the command line). -leif 84 87 return True 85 88 G = abspath(F) 86 89 i = G.rfind(os.path.sep) 90 # XXX The following should IMHO be performed in populatefilelist(): 91 # (Currently, populatefilelist() only looks for "__nodoctest__".) 87 92 if os.path.exists(os.path.join(G[:i], 'nodoctest.py')): 88 93 printmutex.acquire() 89 94 print "%s (skipping) -- nodoctest.py file in directory"%abs(F) 95 sys.stdout.flush() 90 96 printmutex.release() 91 97 return True 92 98 filenm = os.path.split(F)[1] … … 95 101 return True 96 102 if G.find(os.path.join('doc', 'output')) != -1: 97 103 return True 104 # XXX The following is (also/already) handled in populatefilelist(): 98 105 if not (os.path.splitext(F)[1] in ['.py', '.pyx', '.tex', '.pxi', '.sage', '.rst']): 99 106 return True 100 107 return False … … 115 122 for i in range(0,numiteration): 116 123 os.chdir(os.path.dirname(F)) 117 124 command = os.path.join(SAGE_ROOT, 'local', 'bin', 'sage-%s' % cmd) 125 # FIXME: Why call bash here? (Also, we use 'shell=True' below anyway.) 118 126 s = 'bash -c "%s %s > %s" ' % (command, filestr, outfile.name) 119 127 try: 120 128 t = time.time() … … 161 169 print sage_test_cmd(F[len(CUR)+1:]) 162 170 else: 163 171 print abs(F) 172 sys.stdout.flush() 164 173 if ol!="" and (not ol.isspace()): 165 174 if (ol[len(ol)-1]=="\n"): 166 175 ol=ol[0:len(ol)-1] 167 176 print ol 177 sys.stdout.flush() 168 178 time_dict[abs_sage_path(F)] = finished_time 169 179 if XML_RESULTS: 170 180 t = finished_time … … 192 202 """.strip() % locals()) 193 203 f.close() 194 204 print "\t [%.1f s]"%(finished_time) 205 sys.stdout.flush() 195 206 196 207 def infiles_cmp(a,b): 197 208 """ … … 231 242 base, ext = os.path.splitext(F) 232 243 if not (ext in ['.sage', '.py', '.pyx', '.tex', '.pxi', '.rst']): 233 244 continue 234 elif '__nodoctest__' in files: 245 elif '__nodoctest__' in files: # XXX Shouldn't this be 'lfiles'? 246 # Also, this test should IMHO be in the outer loop (1 level). 247 # Furthermore, the current practice is to put "nodoctest.py" 248 # files in the directories that should be skipped, not 249 # "__nodoctest__". (I haven't found a single instance of the 250 # latter in Sage 4.6.1.alpha3.) 251 # "nodoctest.py" is handled in skip() (!), to also be fixed. 252 # -leif 235 253 continue 236 254 appendstr = os.path.join(root,F) 237 255 if skip(appendstr): … … 252 270 argv = [X for X in argv if X[0] != '-'] 253 271 254 272 try: 273 # FIXME: Nice, but <NUMTHREADS> should immediately follow '-tp' etc., 274 # i.e., be the next argument. We might have file or directory 275 # names that properly convert to an int... 255 276 numthreads = int(argv[1]) 256 277 infiles = argv[2:] 257 278 except ValueError: # can't convert first arg to an integer: arg was probably omitted
(This is against Sage 4.6.1.alpha3.)
The comments all refer to inconsistencies; the only actual change is flushing the output since it currently comes in bursts, which is IMHO odd for watching it. I don't think this measurably slows down doctesting...
comment:33 follow-up: ↓ 34 Changed 11 years ago by
Just for the record:
#10458 also patches sage-ptest
to support IPython/Sage-style line continuations in doctests ("....:
" rather than only "...
").
comment:34 in reply to: ↑ 33 Changed 11 years ago by
comment:35 in reply to: ↑ description Changed 11 years ago by
- Cc Marian Rosales added; drkirkby jhpalmieri leif robertwb jdemeyer removed
- Component changed from doctest to build
- Description modified (diff)
- Keywords currency trading forex analysis forex trading online forex trading added; doctest scripts removed
- Priority changed from blocker to trivial
- Summary changed from Handle duplicate file basenames when testing multiple files in parallel to Forex trading - Expert
- Type changed from defect to task
Replying to mpatel:
When we test
/path/to/foo.py
sage-doctest
writes
SAGE_TESTDIR/.doctest_foo.py
runs the new file through
python
, and deletes it. This can cause collisions when we test in parallel multiple files with the same basename, e.g.,__init__
,all
,misc
,conf
,constructor
,morphism
,index
,tests
,homset
,element
,twist
,tutorial
,sagetex
,crystals
,cartesian_product
,template
,ring
, etc.There's a similar problem with testing non-library files, which
sage-doctest
first effectively copies toSAGE_TESTDIR
.See sage-devel for background.
This ticket may help with some of the doctesting problems discussed on the Sage mailing lists. Related tickets: #9224, #9225, #9449.
comment:36 Changed 11 years ago by
- Cc drkirkby jhpalmieri leif robertwb jdemeyer added; Marian Rosales removed
- Component changed from build to doctest
- Description modified (diff)
- Keywords doctest scripts added; currency trading forex analysis forex trading online forex trading removed
- Priority changed from trivial to blocker
- Summary changed from Forex trading - Expert to Handle duplicate file basenames when testing multiple files in parallel
- Type changed from task to defect
comment:37 follow-up: ↓ 38 Changed 11 years ago by
Please ban forextrading11 for spam.
comment:38 in reply to: ↑ 37 Changed 11 years ago by
comment:39 follow-up: ↓ 40 Changed 11 years ago by
- Milestone changed from sage-4.6.2 to sage-4.7
comment:40 in reply to: ↑ 39 Changed 11 years ago by
Replying to jdemeyer:
This is marked as a blocker for 4.7. but there's been no work on it for 8 weeks.
Dave
comment:41 Changed 11 years ago by
- Priority changed from blocker to critical
comment:42 Changed 11 years ago by
- Description modified (diff)
comment:43 Changed 11 years ago by
To me it would -- at least for the moment, or this ticket -- be totally sufficient to have unique filenames by mangling the relative path into the temporary filename.
If someone wants to test multiple Sage installations at the same time, he can simply set SAGE_TESTDIR
to different locations in each shell.
Doctesting in (or below) $DOT_SAGE
(which is usually $HOME/.sage/
) is IMHO a bad idea anyway, not only because /tmp/
is much more likely to be [on] a local filesystem than /home/
. (It is usually also faster and auto-cleaned.) Creating a unique temporary directory there shouldn't be a problem.
For the sake of "For whitespace errors see ...", failing files could still be copied elsewhere (to a perhaps more persistent location) at the end of the doctest process.
comment:44 Changed 11 years ago by
- Description modified (diff)
I've restored the original description the spammer deleted.
comment:45 Changed 11 years ago by
For the record, we already have the following in sage/misc/misc.py
:
... HOSTNAME = socket.gethostname().replace('-','_').replace('/','_').replace('\\','_') LOCAL_IDENTIFIER = '%s.%s'%(HOSTNAME , os.getpid()) ... try: DOT_SAGE = os.environ['DOT_SAGE'] except KeyError: try: DOT_SAGE = '%s/.sage/'%os.environ['HOME'] except KeyError: DOT_SAGE = '%s/.sage/'%SAGE_ROOT ... if not os.path.exists(DOT_SAGE): os.makedirs(DOT_SAGE) _mode = os.stat(DOT_SAGE)[stat.ST_MODE] _desired_mode = 040700 # drwx------ if _mode != _desired_mode: print "Setting permissions of DOT_SAGE directory so only you can read and write it." # Change mode of DOT_SAGE. os.chmod(DOT_SAGE, _desired_mode) ... SAGE_TMP='%s/temp/%s/%s/'%(DOT_SAGE, HOSTNAME, os.getpid()) if not os.path.exists(SAGE_TMP): try: os.makedirs(SAGE_TMP) except OSError, msg: print msg raise OSError, " ** Error trying to create the Sage tmp directory..." ...
comment:46 Changed 11 years ago by
... so we could do almost the same for / below SAGE_TESTDIR
, perhaps creating a single directory name from HOSTNAME
and the PID (avoiding further race conditions, provided $SAGE_TESTDIR
already exists), putting all temporary files (names mangled as proposed, unique to each test instance) into that, i.e.
${SAGE_TESTDIR}/${hostname}-${pid}/doctest-relative__path__to__foo__foo.py
for a file ./relative/path/to/foo/foo.py
.
comment:47 follow-up: ↓ 48 Changed 11 years ago by
Can we just use Python's tempfile module, for example mkstemp? This should produce a temporary file safely, avoiding race conditions. We can have the name end with the file being tested, including its path.
(I think we should do the same thing for SAGE_TMP
in misc.py
, but that's for another ticket.)
comment:48 in reply to: ↑ 47 Changed 11 years ago by
Replying to jhpalmieri:
Can we just use Python's tempfile module, for example mkstemp? This should produce a temporary file safely, avoiding race conditions. We can have the name end with the file being tested, including its path.
I see no reason for doing so. We don't need even more cryptic filenames, and I don't think it will work across NFS filesystems.
The only "race condition" that can occur in what I suggested above is in the creation of SAGE_TESTDIR
itself (if it doesn't already exist), and that can easily be catched.
(I think we should do the same thing for
SAGE_TMP
inmisc.py
, but that's for another ticket.)
I wouldn't do that either. If we create temporary files from shell scripts, we can use the same mechanism.
comment:49 Changed 11 years ago by
Furthermore:
"The file descriptor is not inherited by child processes."
"There is thus no guarantee that the generated filename will have any nice properties, such as not requiring quoting when passed to external commands via
os.popen()
."
comment:50 follow-up: ↓ 52 Changed 11 years ago by
I see no reason for doing so.
There are several reasons for doing so: one is to not reinvent the wheel, and anything we come up with is likely to be less robust than what's built into Python. Also, if we want to doctest outside of $HOME/.sage
, is /tmp
always the best place? The mkstemp function doesn't always create files there, so I'm not convinced we should.
By the way, using the PID in the directory name means creating many directories: as far as I can tell, running sage -t DIR
uses a different process for each file in DIR. Perhaps the PID should be in the mangled filename instead of part of a new directory.
comment:51 Changed 11 years ago by
Still, the best thing is to not create temporary files at all.
(And only dump what would have been written in case of doctest errors, once at the end, with the "same problems", though identical filenames would there be even more unlikely, and could easily be numbered.)
But that's probably for another ticket; we should IMHO here only quickly fix the most prominent errors, namely identical filenames within the same ptest
[long
] instance. If we also allow simultaneous testing of the same or different Sage installations, that's ok, but a pretty minor issue, because -- in contrast to the former -- you can trivially avoid race conditions between such concurrent ptest
runs. (And a normal user wouldn't do the latter either.)
comment:52 in reply to: ↑ 50 ; follow-up: ↓ 53 Changed 11 years ago by
Replying to jhpalmieri:
I see no reason for doing so.
There are several reasons for doing so: one is to not reinvent the wheel, and anything we come up with is likely to be less robust than what's built into Python.
Amen? Also, unless it uses a local filesystem, I don't think it will work with NFS.
Also, if we want to doctest outside of
$HOME/.sage
, is/tmp
always the best place? The mkstemp function doesn't always create files there, so I'm not convinced we should.
It simply (in contrast to Sage) respects the commonly used TMP
(TEMP
on M$ Windows) and TMPDIR
environment variables.
Btw, on typical machines /tmp
does not even really exist on a drive, it's just in memory (and if that's exhausted, swap space will be used transparently). If it is a real partition on a drive, you either use an SSD or at least use that area of a conventional hard drive that is fastest (same for swap).
By the way, using the PID in the directory name means creating many directories: as far as I can tell, running
sage -t DIR
uses a different process for each file in DIR. Perhaps the PID should be in the mangled filename instead of part of a new directory.
At least at the moment, we're dealing with ptest*
only here, so that's another matter (if you want to run multiple testlong
s for example using the same temporary directory). So sage -t ...
wouldn't create any directories at all.
comment:53 in reply to: ↑ 52 ; follow-up: ↓ 54 Changed 11 years ago by
Replying to leif:
By the way, using the PID in the directory name means creating many directories: as far as I can tell, running
sage -t DIR
uses a different process for each file in DIR. Perhaps the PID should be in the mangled filename instead of part of a new directory.At least at the moment, we're dealing with
ptest*
only here, so that's another matter (if you want to run multipletestlong
s for example using the same temporary directory). Sosage -t ...
wouldn't create any directories at all.
I was envisioning patching sage-doctest, since that's what copies the file being tested to SAGE_TESTDIR (and later deletes it), and it gets run by "sage -t ...", "sage -tp ...", etc. Running "sage -tp DIR" also uses a different PID for each execution of sage-doctest.
comment:54 in reply to: ↑ 53 ; follow-up: ↓ 55 Changed 11 years ago by
Replying to jhpalmieri:
I was envisioning patching sage-doctest, since that's what copies the file being tested to SAGE_TESTDIR (and later deletes it), and it gets run by "sage -t ...", "sage -tp ...", etc. Running "sage -tp DIR" also uses a different PID for each execution of sage-doctest.
I would simply pass sage-doctest
the already created directory (either as a parameter, or simply in the environment variable which it already uses anyway).
Creating the directory inside sage-doctest
doesn't make any sense, since all instances would attempt to create it. (You can or could use os.getppid()
though.)
The whole collection of doctest scripts needs an overhaul (or redesign) in the long run...
P.S.: We can use tempfile.gettempdir()
in case SAGE_TESTDIR
isn't set if that makes you happy.
comment:55 in reply to: ↑ 54 Changed 11 years ago by
- Status changed from needs_work to needs_review
Right now, if there are doctest failures or if doctesting is interrupted, the temporary files remain in SAGE_TEMPDIR. If we create a new directory each time, then these directories will remain (we should certainly check if they're empty and then delete them, but if they're not empty, we should not automatically delete them, I think). Is it any better or worse to have a bunch of directories in SAGE_TEMPDIR, as opposed to a bunch of files?
Meanwhile, here's an attempt at a patch.
comment:56 follow-up: ↓ 57 Changed 11 years ago by
At this point just a comment on the naming / mangling:
- To me, it doesn't make any sense to create "hidden" files, so I would omit the leading period.
- We should perhaps apply some character substitutions to
socket.gethostname()
as well, likesage.misc.misc
does (see quote above). - Thinking more about it, I would replace slashs (
os.path.sep
) by periods (and not double-underscores or, as currently, dashs), since that way the temporary filenames resemble (at least partially) fully qualified Python module names.
(As discussed, I'd also change the default for SAGE_TESTDIR
, e.g. to tempfile.gettempdir()
.)
We could make sure once that the temporary directory (${SAGE_TESTDIR}/${hostname}-${pid}/
) is writable by the user. I think we should also clean it up in case it already exists, as anything left there is potentially very old and unrelated to the current test run. We might also adjust the permissions of the directory.
Looking only at your (John's) patch, is temp_py_file
defined anywhere? (I only see it gets added to tmpfiles
.)
P.S.: Why not base the patch on Mitesh's? (Which AFAIR only needed two changes.)
comment:57 in reply to: ↑ 56 Changed 11 years ago by
Replying to leif:
At this point just a comment on the naming / mangling:
- To me, it doesn't make any sense to create "hidden" files, so I would omit the leading period.
Okay, not a high priority and outside the scope of this ticket, but harmless enough. Done. (We could do the same thing with the files recording timing data, but that's for another ticket.)
- We should perhaps apply some character substitutions to
socket.gethostname()
as well, likesage.misc.misc
does (see quote above).
Done.
- Thinking more about it, I would replace slashs (
os.path.sep
) by periods (and not double-underscores or, as currently, dashs), since that way the temporary filenames resemble (at least partially) fully qualified Python module names.
Done.
(As discussed, I'd also change the default for
SAGE_TESTDIR
, e.g. totempfile.gettempdir()
.)
I know that I was suggesting this earlier, but on further reflection, we shouldn't do this here: for example, there may be users who expect the doctesting files to be in .sage/tmp/
, and changing this may therefore make people unhappy. I think it's a good idea, but it should be on another ticket. (I'm agreeing with Dave's point that we should try to fix just the bug here, and then we can work on other improvement separately.)
We could make sure once that the temporary directory (
${SAGE_TESTDIR}/${hostname}-${pid}/
) is writable by the user. I think we should also clean it up in case it already exists, as anything left there is potentially very old and unrelated to the current test run. We might also adjust the permissions of the directory.
Done.
Looking only at your (John's) patch, is
temp_py_file
defined anywhere? (I only see it gets added totmpfiles
.)
That was a mistake, which I think I've fixed.
P.S.: Why not base the patch on Mitesh's? (Which AFAIR only needed two changes.)
No good reason.
comment:58 follow-up: ↓ 60 Changed 11 years ago by
Oops, just found a mistake. In non-Sage library code, when doctesting "file0.py", we write a line
from file0 import *
With the name mangling, this doesn't work anymore: the periods confuse things, and so would hyphens, commas, and other symbols not allowed in python module names. We can either just revert the part of the code involving non-Sage library code, or we can try to fix the mangled names. Right now I'm doing the first of these, since the first priority should be to fix things for doctesting the Sage library.
comment:59 Changed 11 years ago by
Replying to jhpalmieri:
Replying to leif:
(As discussed, I'd also change the default for
SAGE_TESTDIR
, e.g. totempfile.gettempdir()
.)I know that I was suggesting this earlier, but on further reflection, we shouldn't do this here: for example, there may be users who expect the doctesting files to be in
.sage/tmp/
, and changing this may therefore make people unhappy. I think it's a good idea, but it should be on another ticket. (I'm agreeing with Dave's point that we should try to fix just the bug here, and then we can work on other improvement separately.)
That was my opinion; Dave earlier requested extensions to allow simultaneous testing (including make test
[long
] etc.) of multiple (possibly the same) Sage installations in the "same" directory (SAGE_TESTDIR
), i.e. without having to manually set SAGE_TESTDIR
in different shells.
As further enhancements, we should perhaps print the location at the beginning, and also at the end if any doctest errors occurred. (I'm right now not sure if GLOBAL_ITER
etc. get printed, I only recall we should also print either SAGE_TIMEOUT
or SAGE_TIMEOUT_LONG
, whichever is appropriate, as many people seem to misunderstand the meaning and perhaps also don't know the defaults, which are btw. wall time and not CPU time -- perhaps subject to change later as well... :) .)
(A few print
statements wouldn't complicate the patch here though. And if we print SAGE_TESTDIR
anyway, we could at the same time change its default to a sensible value as well.)
P.S.: Why not base the patch on Mitesh's? (Which AFAIR only needed two changes.)
No good reason.
Hmmm. Mitesh fixed some other things as well, so we should somehow make sure the changes don't get lost once this ticket is merged / closed. (I remember having reviewed them last year, before he updated the patch again though IIRC. I think Robert was also ok with his changes, except for the creation of directories / the temporary files' names and locations.)
Unfortunately, there are meanwhile many "concurrent" tickets dealing with the doctest scripts.
I'd of course also like to see my comments from the inline patch to `sage-ptest` above in it... ;-) (Just the comments, which are FIXMEs / TODOs, not [necessarily] the flush()
s.)
I'll apply and take a look at your second patch later.
comment:60 in reply to: ↑ 58 ; follow-up: ↓ 61 Changed 11 years ago by
Replying to jhpalmieri:
Oops, just found a mistake. In non-Sage library code, when doctesting "file0.py", we write a line
from file0 import *
With the name mangling, this doesn't work anymore: the periods confuse things, and so would hyphens, commas, and other symbols not allowed in python module names.
Hence underscores, which I originally thought of?
It doesn't make sense to copy [only] each single non-library file to doctest to the temporary directory anyway, as it might import other files located in the original directory.
We could either just cd
to the original directory, or -- IMHO much better -- add the original directory to PYTHONPATH
or sys.path
, both methods solving the mangling issue in from ... import *
, as well as saving useless copying. (We of course still have to create temporary preparsed files in SAGE_TESTDIR/.../
for .sage
and .spyx
files though.)
comment:61 in reply to: ↑ 60 ; follow-up: ↓ 62 Changed 11 years ago by
Replying to leif:
Replying to jhpalmieri:
Oops, just found a mistake. In non-Sage library code, when doctesting "file0.py", we write a line
from file0 import *With the name mangling, this doesn't work anymore: the periods confuse things, and so would hyphens, commas, and other symbols not allowed in python module names.
Hence underscores, which I originally thought of?
Well, the host name could contain all sorts of characters in it, couldn't it? Same for the directories in the path to the file, especially since we're talking about files not in the Sage library. Doing some sort of regexp search and replace is a lot of work for perhaps minimal gain. It certainly doesn't have to do with the issue on this ticket.
It doesn't make sense to copy [only] each single non-library file to doctest to the temporary directory anyway, as it might import other files located in the original directory.
Outside the scope of this ticket. If we leave that part alone, we're not creating a new bug, just leaving a less-than-perfect implementation in place.
Meanwhile, if you want to add in some print statements, comments, some of the relevant parts of Mitesh's patch, or anything else, go ahead. I have to work on some other things for at least a few days.
comment:62 in reply to: ↑ 61 Changed 11 years ago by
Replying to jhpalmieri:
Replying to leif:
Replying to jhpalmieri:
Oops, just found a mistake. In non-Sage library code, when doctesting "file0.py", we write a line
from file0 import *
With the name mangling, this doesn't work anymore: the periods confuse things, and so would hyphens, commas, and other symbols not allowed in python module names.
Hence underscores, which I originally thought of?
Well, the host name could contain all sorts of characters in it, couldn't it? Same for the directories in the path to the file, especially since we're talking about files not in the Sage library. Doing some sort of regexp search and replace is a lot of work for perhaps minimal gain.
Well, you don't have to mangle the name in the import
statement at all (and the hostname is part of the directory name, not a filename, anyway).
It certainly doesn't have to do with the issue on this ticket.
Of course it does. You tried to also mangle the import
name to avoid name clashs, but that's simply not necessary. Just prepend the original (source) directory of the file to test to PYTHONPATH
, and keep the [base]name of the file in from ... import *
unmodified (and of course without a path).
For files of the Sage library, we strip that path (at least Mitesh did), since all necessary (root) directories are already in PYTHONPATH
.
The only "problem" are files that have to be preparsed (*.sage
), at least if we don't want to create the intermediate, preparsed .py
files at the original location, preferably just once.
For these files, we simply can do almost what Mitesh did, namely create a temporary file with an "arbitrary" (Python module name-conforming) but unique basename and the extension .py
in SAGE_TESTDIR
(from the perspective of sage-doctest
, i.e. already containing the hostname and the pid of the parent process), using either tempfile.mkstemp()
[which is safe here] or preferably just sage-doctest
's PID, import that in the doctest_*
file, and also append it to tmpfiles
:
# We are in "sage-doctest", # SAGE_TESTDIR is already ".../${hostname}-${ppid}/" ... if not library_code: if ext in ['.pyx','.spyx']: s += "cython(open('%s').read())\n\n" % file_name elif ext in ['.py', '.sage']: source = file_name # full name with path target_name = "%s_%d" % (name, os.getpid()) # like 'name', but unique target_base = os.path.join(SAGE_TESTDIR, target_name) # like 'base', also unique if ext == '.sage': # Unfortunately, "sage -preparse <file>.sage" doesn't have any # output options and always creates <file>.py in the same # directory, so we first copy the *source* into SAGE_TESTDIR: os.system("cp '%s' %s.sage" % (source, target_base)) # Now create SAGE_TESTDIR/<target_name>.py: os.system("sage -preparse %s.sage" % target_base) # Remove the copy of the original (.sage): # (We could also just add it to 'tmpfiles'.) os.system("rm -f %s.sage" % target_base) else: # Plain Python file (".py"), just copy it: # (If we added source's directory to PYTHONPATH, # we wouldn't have to copy the file, but then also # would have to import from 'name' instead of 'target_name'.) os.system("cp '%s' %s.py" % (source, target_base)) s += "from %s import *\n\n" % target_name tmpfiles.append(target_base + ".py") # preparsed or copied original tmpfiles.append(target_base + ".pyc") # compiled version of it ...
A better solution, as Mitesh noted in the TODO
comment, is to preparse the file into a string, and directly write that string into the doctest_*
file where we currently have the from ... import *
.
That enhancement is most probably a thing to do on a follow-up ticket, but not supporting (i.e. avoiding name clashes for) non-library files would be a regression w.r.t. Mitesh's patch.
It doesn't make sense to copy [only] each single non-library file to doctest to the temporary directory anyway, as it might import other files located in the original directory.
Outside the scope of this ticket. If we leave that part alone, we're not creating a new bug, just leaving a less-than-perfect implementation in place.
See above. I there just copy the "main" file, which we still can change later.
Meanwhile, if you want to add in some print statements, comments, some of the relevant parts of Mitesh's patch, or anything else, go ahead. I have to work on some other things for at least a few days.
As I said, I would have preferred having your patch based on Mitesh's, since there are a couple of changes that could have been kept, just changing the "mangling".
I can add a reviewer patch for my comments (and perhaps a few print
statements) later; rebasing Mitesh's would be a lot of work and so I don't know if I'll do that.
comment:63 follow-up: ↓ 65 Changed 11 years ago by
Here is a patch based on Mitesh's; the "delta" patch shows the changes; these changes include your in-line patch. This does not change SAGE_TESTDIR to tempfile.gettempdir()
: we store timing information in this directory, so it should not be temporary. It might be a good idea to change TMP
(as used in sage-ptest
) to a temporary directory, but I didn't make this change either, just added a comnent about it.
The changes:
- the filename mangling uses the full path of the file rather than
tempfile.mkstemp
; this should be good enough, especially since we work in a directory with name determined by the pid and the hostname. - there was one situation in which the temporary files stored in
tmpfiles
needed to be deleted but weren't; this was the case before any of the patches. - there are now messages printed about the doctesting directory, and then deleting it at the end.
comment:64 Changed 11 years ago by
Oh, I forgot: I tested this with some non-library code, and it mostly worked. I say "mostly" because when I set SAGE_CHECK=yes
and installed the package at #9894, a bunch of files produced errors. I got the following odd behavior:
- before applying the patch, after spkg-check ran, it said that maybe a dozen files gave errors, but only 4 of them remained in
SAGE_TESTDIR
(4 of them, plus their.pyc
files, plus their.doctest...
files).
- after applying the patch: for the dozen files which gave errors, all of them remained in
SAGE_TESTDIR
, plus their.pyc
files, plus only 4 of thedoctest...
files — the ones corresponding to the same 4 files as in the first case.
I don't know what's going on here. The spkg-check script is not a completely straightforward program; it doesn't just run "sage -tp" on some directory. So that may cause some of the issues. Anyway, there are better results after applying the patch than before, but it's still not perfect.
For all of my other tests with non-library code, it worked just the way it should.
comment:65 in reply to: ↑ 63 ; follow-up: ↓ 66 Changed 11 years ago by
Sorry, atm too tired to look at the whole, only a few remarks before I again forget them...
Replying to jhpalmieri:
Here is a patch based on Mitesh's; the "delta" patch shows the changes; these changes include your in-line patch.
Nice, thanks. Even flushing the output.
This does not change SAGE_TESTDIR to
tempfile.gettempdir()
: we store timing information in this directory, so it should not be temporary.
Who cares? ;-)
The odd thing with that is that we then again produce potential race conditions for the timing files. I think we'd have to use locking then (which can also cause headaches), perhaps on a follow-up.
IMHO these files also shouldn't be "hidden", and could live in or below DOT_SAGE
(if we have to use locking anyway). Moreover, they perhaps shouldn't get lost if one "manually" sets SAGE_TESTDIR
to e.g. /tmp
, which seems reasonable at least as long as we don't automatically use some presumably fast filesystem for the really temporary files.
It might be a good idea to change
TMP
(as used insage-ptest
) to a temporary directory, but I didn't make this change either, just added a comnent about it.
Ok, see above; at least documented.
The changes:
- the filename mangling uses the full path of the file rather than
tempfile.mkstemp
; this should be good enough, especially since we work in a directory with name determined by the pid and the hostname.
This doesn't help when simultaneously testing the same file from one sage-ptest
instance (SAGE_TEST_ITER
, SAGE_TEST_GLOBAL_ITER
) if I'm not wrong; we could mangle sage-doctest
's PID into the filename, too, as I suggested above.
- there are now messages printed about the doctesting directory, and then deleting it at the end.
For readability, I'd move the print
statements ("Removing the directory ...", which hopefully don't raise exceptions) out of the try
block.
Also, sage-ptest
should know whether tests failed (or doctesting was interrupted), in which case we don't have to issue a warning since keeping the failed doctest files is intentional (and the left files should have been mentioned in previous messages).
So I wouldn't try to remove the directory if any doctests failed, unless they were [all] interrupted by Ctrl-C (in which case sage-doctest
should immediately remove all temporary files belonging to the aborted doctest, which it currently doesn't).
Btw., unrelated to this ticket: sage-doctest
shouldn't sleep for 0.1 seconds (and not continually poll the state of the child process) if the timeout is in seconds anyway; instead, it should use signal.alarm()
and wait()
.
comment:66 in reply to: ↑ 65 ; follow-up: ↓ 67 Changed 11 years ago by
Replying to leif:
This does not change SAGE_TESTDIR to
tempfile.gettempdir()
: we store timing information in this directory, so it should not be temporary.Who cares? ;-)
Well, someone might...
The odd thing with that is that we then again produce potential race conditions for the timing files. I think we'd have to use locking then (which can also cause headaches), perhaps on a follow-up.
True, theoretically, but I've never heard of this coming up. I don't think these files are open for very long, so race conditions should be very rare. I agree it should be dealt with on a follow-up, if at all. (Instead of locking, we could perhaps use a "try ... except" block, since if two processes are trying to write to the same timing file, it's not a disaster if we just completely discard one of those.)
IMHO these files also shouldn't be "hidden", and could live in or below
DOT_SAGE
I thought about this when working on the most recent patch, but it was too much work for too little gain to make it backwards compatible (if .ptest_timing...
exists, then read it, otherwise, look for ptest_timing...
, etc.). It could be done on another ticket.
(if we have to use locking anyway). Moreover, they perhaps shouldn't get lost if one "manually" sets
SAGE_TESTDIR
to e.g./tmp
, which seems reasonable at least as long as we don't automatically use some presumably fast filesystem for the really temporary files.
That's a good point.
The changes:
- the filename mangling uses the full path of the file rather than
tempfile.mkstemp
; this should be good enough, especially since we work in a directory with name determined by the pid and the hostname.This doesn't help when simultaneously testing the same file from one
sage-ptest
instance (SAGE_TEST_ITER
,SAGE_TEST_GLOBAL_ITER
) if I'm not wrong; we could manglesage-doctest
's PID into the filename, too, as I suggested above.
I can do that, or we can use mkstemp
instead of or in addition to the path. Opinions either way?
- there are now messages printed about the doctesting directory, and then deleting it at the end.
For readability, I'd move the
try
block.
Okay.
Also,
sage-ptest
should know whether tests failed (or doctesting was interrupted), in which case we don't have to issue a warning since keeping the failed doctest files is intentional (and the left files should have been mentioned in previous messages).So I wouldn't try to remove the directory if any doctests failed, unless they were [all] interrupted by Ctrl-C (in which case
sage-doctest
should immediately remove all temporary files belonging to the aborted doctest, which it currently doesn't).
Perhaps it should, but that should be on another ticket. As Dave said earlier, "a sub-optimal solution is a better temporary measure than a complete industrial strength bullet-proof solution". I don't want to have to deal with the inner workings of doctesting here any more than is necessary, and I don't think this particular issue is necessary to deal with for this ticket. I can add a comment to the code about this.
comment:67 in reply to: ↑ 66 ; follow-up: ↓ 68 Changed 11 years ago by
- Reviewers changed from Robert Bradshaw to Robert Bradshaw, Leif Leonhardy
Replying to jhpalmieri:
True, theoretically, but I've never heard of this coming up. I don't think these files are open for very long, so race conditions should be very rare. I agree it should be dealt with on a follow-up, if at all. (Instead of locking, we could perhaps use a "try ... except" block, since if two processes are trying to write to the same timing file, it's not a disaster if we just completely discard one of those.)
Ok, I thought timings were accumulated; never looked at this.
I thought about this when working on the most recent patch, but it was too much work for too little gain to make it backwards compatible (if
.ptest_timing...
exists, then read it, otherwise, look forptest_timing...
, etc.). It could be done on another ticket.
I wouldn't care much about backward compatibility in this case.
- the filename mangling uses the full path of the file rather than
tempfile.mkstemp
; this should be good enough, especially since we work in a directory with name determined by the pid and the hostname.This doesn't help when simultaneously testing the same file from one
sage-ptest
instance (SAGE_TEST_ITER
,SAGE_TEST_GLOBAL_ITER
) if I'm not wrong; we could manglesage-doctest
's PID into the filename, too, as I suggested above.I can do that, or we can use
mkstemp
instead of or in addition to the path. Opinions either way?
I'd prefer sage-doctest
's PID, appended (separated by an underscore) as above.
Also,
sage-ptest
should know whether tests failed (or doctesting was interrupted), in which case we don't have to issue a warning since keeping the failed doctest files is intentional (and the left files should have been mentioned in previous messages).
So I wouldn't try to remove the directory if any doctests failed, unless they were [all] interrupted by Ctrl-C (in which casesage-doctest
should immediately remove all temporary files belonging to the aborted doctest, which it currently doesn't).Perhaps it should, but that should be on another ticket.
The deletion upon Ctrl-C in sage-doctest
; I just wouldn't try to remove the directory in sage-ptest
if any error occurred (along with a perhaps confusing warning message).
"complete industrial strength bullet-proof solution"
I don't think we'll ever reach this, also because the addition of new features will never stop. So Sage's version number won't converge.
I can add a comment to the code about this.
I'd appreciate that, such that others can catch up. IMHO too much things end up in ticket comments hardly anybody will see or read later.
comment:68 in reply to: ↑ 67 ; follow-up: ↓ 70 Changed 11 years ago by
Replying to leif:
I'd prefer
sage-doctest
's PID, appended (separated by an underscore) as above.
Do we also need to add this PID to the output from filename_mangler
, in case someone is doctesting the same file in the Sage library several times simultaneously?
Regarding the timing files: I'm not touching that code, although I've added some comments.
Regarding deleting files on interruption: it's not clear how to easily determine whether there was an error before the interruption — do you need to use post_process
? — so I'm just adding some comments and not touching the code.
Changed 11 years ago by
comment:69 Changed 11 years ago by
- Description modified (diff)
comment:70 in reply to: ↑ 68 ; follow-up: ↓ 72 Changed 11 years ago by
Replying to jhpalmieri:
Replying to leif:
I'd prefer
sage-doctest
's PID, appended (separated by an underscore) as above.Do we also need to add this PID to the output from
filename_mangler
, in case someone is doctesting the same file in the Sage library several times simultaneously?
Yes; you apparently already did so in the new patch.
But we actually don't have to mangle any path into the filename if we do that, since every file is tested by its own sage-doctest
instance, in a directory unique to the host and the sage-ptest
instance. [We'd have to do the same for sage-test
, i.e., also create a unique directory there, perhaps on a follow-up, to also support simultaneous sequential testing on different hosts which share the same $SAGE_TESTDIR
(from the perspective of sage-[p]test
).]
Regarding the timing files: I'm not touching that code, although I've added some comments.
Ok.
Regarding deleting files on interruption: it's not clear how to easily determine whether there was an error before the interruption — do you need to use
post_process
? — so I'm just adding some comments and not touching the code.
Ok. I actually didn't think about doctest errors in the same file that may have occurred prior to interruption; it would IMHO be ok to just "discard" them, but we can decide on that on the corresponding follow-up.
comment:71 Changed 11 years ago by
P.S.: For future changes, could we do some versioning with the patch(es)?
I meanwhile have a handful of tabs with different versions of the attached one and different deltas, losing track... ;-)
comment:72 in reply to: ↑ 70 ; follow-up: ↓ 74 Changed 11 years ago by
What still needs to be done here?
Replying to leif:
Replying to jhpalmieri:
Replying to leif:
I'd prefer
sage-doctest
's PID, appended (separated by an underscore) as above.Do we also need to add this PID to the output from
filename_mangler
, in case someone is doctesting the same file in the Sage library several times simultaneously?Yes; you apparently already did so in the new patch.
But we actually don't have to mangle any path into the filename if we do that, since every file is tested by its own
sage-doctest
instance, in a directory unique to the host and thesage-ptest
instance.
I can remove the pathname from the mangling. If we're just adding the pid, I may discard the function filename_mangler
and deal with it like this:
- f = os.path.splitext(filename_mangler(file))[0] + '.py' + f = os.path.join(SAGE_TESTDIR, "doctest_%s_%s.py" % (os.getpid(), name))
filename_mangler
only gets used in this one place anyway.
Regarding deleting files on interruption: it's not clear how to easily determine whether there was an error before the interruption — do you need to use
post_process
? — so I'm just adding some comments and not touching the code.Ok. I actually didn't think about doctest errors in the same file that may have occurred prior to interruption; it would IMHO be ok to just "discard" them, but we can decide on that on the corresponding follow-up.
Well, some library files can take a long time to doctest, so I can imagine someone doctesting a file, seeing that it fails and then interrupting it, but wanting to keep the temporary file for some reason. (As I said, I'm not planning on modifying this code anyway.)
comment:73 Changed 11 years ago by
- Priority changed from critical to blocker
Because, as mentioned, it currently can happen that doctests for a file .../bar/foo
are reported to have passed successfully though actually .../baz/foo
was tested.
Afterwards rerunning (just) the tests for .../baz/foo
(which in contrast were reported to have failed) won't show any errors, hiding possible doctest errors in .../bar/foo
.
comment:74 in reply to: ↑ 72 Changed 11 years ago by
Replying to jhpalmieri:
What still needs to be done here?
Replying to leif:Replying to jhpalmieri:
Replying to leif:
I'd prefer
sage-doctest
's PID, appended (separated by an underscore) as above.I can remove the pathname from the mangling. If we're just adding the pid, I may discard the function
filename_mangler
and deal with it like this:
- f = os.path.splitext(filename_mangler(file))[0] + '.py' + f = os.path.join(SAGE_TESTDIR, "doctest_%s_%s.py" % (os.getpid(), name))
I'd prefer having the name first, then the PID; then we can also drop the doctest_
prefix (because e.g. 1_module
is not a valid Python module name).
I think this way it's easier to locate a file (with ls
or some file manager), since the files will be in alphabetical order, sorted by their original name (as opposed to some random PIDs).
comment:75 Changed 11 years ago by
- Description modified (diff)
Here are some new patches. First, trac_9739.v2.patch and a "delta" patch trac_9739-delta1to2.patch: the differences from the previous version are
- It changes how filenames are mangled: it uses "NAME_PID.py" instead of "doctest_PID_NAME.py" or whatever what there before.
- I've decided that I agree with you: we shouldn't print the messages about removing the doctesting directory every time. I've changed it so it only prints them if "-verbose" is one of the command-line options. (This is overusing the "-verbose" option, I suppose, so we could instead could completely disable printing just by setting the variable "verbose" in sage-ptest to be False always.) If the directory is not deleted at the end, then a message is printed regardless of verbosity settings.
Along these lines, we have trac_9739-graphviz.patch, a patch for the main Sage library, which writes a temporary file to SAGE_TMP rather than to SAGE_TESTDIR, so that the doctesting directory is indeed empty after doctesting the Sage library.
William Stein suggests: