Flaky doctest in sage/interfaces/r.py

[mpatel]

This test in src/sage/interfaces/r.py should be fixed:

            sage: os.path.realpath(tmpdir) == sageobj(r.getwd())  # known bug (:trac:`9970`)
            True

The script

import os
import tempfile

fail = 0
for i in xrange(1000):
    tmpdir = tempfile.mkdtemp()
    r.chdir(tmpdir)
    if tmpdir not in sageobj(r.getwd()):
        print tmpdir, r.getwd(), sageobj(r.getwd())
        fail += 1
    os.rmdir(tmpdir)  # clean up

print fail

prints

/tmp/tmpb1LEIM [1] "/tmp/tmpb1LEIM" /tmp/tmpbInteger(1)EIM
/tmp/tmpZ6LyPm [1] "/tmp/tmpZ6LyPm" /tmp/tmpZInteger(6)yPm
/tmp/tmp2LTCse [1] "/tmp/tmp2LTCse" /tmp/tmpInteger(2)TCse
/tmp/tmpzoor1L [1] "/tmp/tmpzoor1L" /tmp/tmpzoorInteger(1)
/tmp/tmpCl70Lo [1] "/tmp/tmpCl70Lo" /tmp/tmpClInteger(70)o
/tmp/tmpN7L_vQ [1] "/tmp/tmpN7L_vQ" /tmp/tmpNInteger(7)_vQ
/tmp/tmp9LAnw8 [1] "/tmp/tmp9LAnw8" /tmp/tmpInteger(9)Anw8
/tmp/tmpvL13LQ [1] "/tmp/tmpvL13LQ" /tmp/tmpvLInteger(13)Q
/tmp/tmpfB9L8g [1] "/tmp/tmpfB9L8g" /tmp/tmpfBInteger(9)8g
/tmp/tmp0LtlQ6 [1] "/tmp/tmp0LtlQ6" /tmp/tmpInteger(0)tlQ6
/tmp/tmpQF4L5I [1] "/tmp/tmpQF4L5I" /tmp/tmpQFInteger(4)5I
/tmp/tmp_0LOOr [1] "/tmp/tmp_0LOOr" /tmp/tmp_Integer(0)OOr
/tmp/tmpdDDj7L [1] "/tmp/tmpdDDj7L" /tmp/tmpdDDjInteger(7)
/tmp/tmpr0LiLu [1] "/tmp/tmpr0LiLu" /tmp/tmprInteger(0)iLu
/tmp/tmpu5LnMJ [1] "/tmp/tmpu5LnMJ" /tmp/tmpuInteger(5)nMJ
/tmp/tmp0Py04L [1] "/tmp/tmp0Py04L" /tmp/tmp0PyInteger(04)
/tmp/tmp3LZjCk [1] "/tmp/tmp3LZjCk" /tmp/tmpInteger(3)ZjCk
17

on sage.math.

[kcrisman]

That's very interesting, thanks for pointing it out. Since the r.chdir() is a pretty recent addition, it's not surprising there are unusual bugs with this. I get the same behavior on Mac OS X 10.4, so this is not platform dependent, unsurprisingly.

I am wondering whether it is simply that the directory names are preparsed or something with all those Integer things - maybe these 13-20 out of 1000 are the ones that have numeric characters in the alphanumeric string?. Though /tmp/tmp0Py04L only has the 04 in Integer, not the first 0. Also, I get

/tmp/tmpooH_dP [1] "/private/tmp/tmpooH_dP" /private/tmp/tmpooH_dP
/tmp/tmpLPmXNF [1] "/private/tmp/tmpLPmXNF" /private/tmp/tmpLPmXNF
/tmp/tmpH7BqRp [1] "/private/tmp/tmpH7BqRp" /private/tmp/tmpH7BqRp
/tmp/tmpmV9yGJ [1] "/private/tmp/tmpmV9yGJ" /private/tmp/tmpmV9yGJ
/tmp/tmpZig4LH [1] "/private/tmp/tmpZig4LH" /private/tmp/tmpZigInteger(4)H

in printing out the first ones until I get a failure (change the if loop to a while loop, basically), and a second time

/tmp/tmp2Ny1fm [1] "/private/tmp/tmp2Ny1fm" /private/tmp/tmp2Ny1fm
/tmp/tmpY6qCbW [1] "/private/tmp/tmpY6qCbW" /private/tmp/tmpY6qCbW
/tmp/tmpQWhSyG [1] "/private/tmp/tmpQWhSyG" /private/tmp/tmpQWhSyG
/tmp/tmpJnST6Z [1] "/private/tmp/tmpJnST6Z" /private/tmp/tmpJnST6Z
/tmp/tmpiP3g5g [1] "/private/tmp/tmpiP3g5g" /private/tmp/tmpiP3g5g
/tmp/tmpBo_DwU [1] "/private/tmp/tmpBo_DwU" /private/tmp/tmpBo_DwU
/tmp/tmp0O2kjX [1] "/private/tmp/tmp0O2kjX" /private/tmp/tmp0O2kjX
/tmp/tmpJzFFFs [1] "/private/tmp/tmpJzFFFs" /private/tmp/tmpJzFFFs
/tmp/tmpF3eRCC [1] "/private/tmp/tmpF3eRCC" /private/tmp/tmpF3eRCC
/tmp/tmp3Vd1Lg [1] "/private/tmp/tmp3Vd1Lg" /private/tmp/tmp3VdInteger(1)g

so that can't be the issue, at least not by itself. The position of the integer seems irrelevant as well.

[mpatel]

Using str instead of sageobj seems to work. Of course, that doesn't explain the behavior of sageobj here.

Aside: Would we avoid the platform-dependent /private prefix with sage.misc.misc.tmp_dir? The sage-cleaner would later delete the directory.

[mpatel]

Ticket #10264 is an apparent duplicate. Jeroen Demeyer has attached a patch there.

[mpatel]

Replying to mpatel:

Ticket #10264 is an apparent duplicate. Jeroen Demeyer has attached a patch there.

No, it seems to be a different problem with the same doctest.

[jdemeyer]

Replying to mpatel:

No, it seems to be a different problem with the same doctest.

Exactly. However, in order not to get into merge conflicts, I propose that any patch for this ticket actually goes to #10264.

[jdemeyer]

If you look carefully, you notice that these errors happen exactly when there is a digit followed by the letter "L".

[kcrisman]

Replying to jdemeyer:

If you look carefully, you notice that these errors happen exactly when there is a digit followed by the letter "L".

Hah! You got it nailed -

/tmp/tmp3Vd1Lg [1] "/private/tmp/tmp3Vd1Lg" /private/tmp/tmp3VdInteger(1)g

the 1L becomes Integer(1). And all of them do that.

So it thinks we have a 'long integer' when in fact it's just a random directory. I think maybe this is a problem in R itself, in setwd?

[jdemeyer]

Replying to kcrisman:

I think maybe this is a problem in R itself, in setwd?

I doubt it, since r.getwd() returns the right thing, but sageobj(r.getwd()) not.

[mpatel]

If I comment out line 1717 in interfaces/r.py

        # Change 'dL' to 'Integer(d)'
        exp = rel_re_integer.sub(self._subs_integer, exp)

(in sage.interfaces.r.RElement._sage_), I get no failures with the script in the description.

[jdemeyer]

Replying to mpatel:

If I comment out line 1717 in interfaces/r.py

        # Change 'dL' to 'Integer(d)'
        exp = rel_re_integer.sub(self._subs_integer, exp)

(in sage.interfaces.r.RElement._sage_), I get no failures with the script in the description.

But I suppose you might get errors somewhere else?

[mpatel]

Replying to jdemeyer:

Replying to mpatel:

If I comment out line 1717 in interfaces/r.py

        # Change 'dL' to 'Integer(d)'
        exp = rel_re_integer.sub(self._subs_integer, exp)

(in sage.interfaces.r.RElement._sage_), I get no failures with the script in the description.

But I suppose you might get errors somewhere else?

You're probably right. I was just following up on your observation about long integers with an experiment.

How about

sage: '"%s"' % os.path.realpath(tmpdir) == r.eval('dput(%s)' % r.getwd().name())
True

? Can we simplify this?

[mpatel]

Or

sage: os.path.realpath(tmpdir) in str(r.getwd())
True

?

[jdemeyer]

Personally, I am against changing doctests when the underlying functions are broken. In this case, it is clearly the conversion from R to Sage which is broken, not the doctest. By changing the doctest, we just hide the bug under the carpet instead of solving it.

[leif]

Replying to jdemeyer:

Personally, I am against changing doctests when the underlying functions are broken. In this case, it is clearly the conversion from R to Sage which is broken, not the doctest. By changing the doctest, we just hide the bug under the carpet instead of solving it.

+1, though the doctest tries to test something else as I understand it.

[wjp]

I have a patch that should fix this, but due to lack of knowledge of R I can't effectively test it. I think it needs some more doctests for the _sage_ function.

[kcrisman]

Replying to wjp:

I have a patch that should fix this, but due to lack of knowledge of R I can't effectively test it. I think it needs some more doctests for the _sage_ function.

I will eventually look at this, but because it's a rather harmless doctest failure (even though we really don't like doctest failures) I won't be able to check it out very quickly.

I'm also putting your request from sage-devel here for reference as to what you think is needed for review - thanks!

If somebody more familiar with the R language and interface could take a look 
if the string-related corner cases work, and if it didn't break anything 
non-string-related (and maybe add some more doctests to the _sage_ method), 
that would be great. 

[jdemeyer]

This is still a very annoying bug.

[SimonKing]

By #12415, a remark was added in r.py, pointing to this ticket:

            sage: os.path.realpath(tmpdir) == sageobj(r.getwd())  # known bug (:trac:`9970`)
            True

Note that in #12876, Nicolas changes the test by inserting os.path.realpath on the right side as well, hence:

            sage: os.path.realpath(tmpdir) == os.path.realpath(sageobj(r.getwd()))
            True

I don't know if this test is all what this ticket is about. So, please decide yourself if this ticket became a duplicate (or actually a sub-problem) of #12876

[wjp]

Unrelated as far as I can tell.

[SimonKing]

Replying to wjp:

Unrelated as far as I can tell.

OK, then the ticket status should probably reverted (it has been "needs work").

In any case, it seems that #12876 needs this change to make the doctest failure vanish. I suggest that a "todo" is added, referring to this ticket, stating that the doctest fix from #12876 is not a fix of the underlying problem, which is dealt with here.

[jdemeyer]

Replying to SimonKing:

In any case, it seems that #12876 needs this change to make the doctest failure vanish.

Are you sure, because I don't see what #12876 has to do with either the conversion from R to Sage or with filesystem paths.

[SimonKing]

Replying to jdemeyer:

Replying to SimonKing:

In any case, it seems that #12876 needs this change to make the doctest failure vanish.

Are you sure, because I don't see what #12876 has to do with either the conversion from R to Sage or with filesystem paths.

Well, at least it used to need it. If I recall correctly, at the time when I created the first version of this patch, we got a consistent error in the test, and with the patch the error consistently vanished.

However, as I've learned today, the tests also pass without this patch. So, we should drop it.

[mmezzarobba]

Replying to SimonKing:

However, as I've learned today, the tests also pass without this patch. So, we should drop it.

So should this ticket be closed as invalid?

[jdemeyer]

Replying to mmezzarobba:

So should this ticket be closed as invalid?

NO

[novoselt]

As of Sage 8.5 I no longer get errors from the script above, so presumably "known bug" can be removed.

However, r.chdir and os.chdir now do the same thing, i.e. the current directory for R and Sage is the same, which suggests to me that this function should be deprecated entirely. Unless it is desirable to be able to set different directories, in which case this has to be reimplemented.

[novoselt]

Timo - am I right that with rpy2 interface the working directory of python and R are always the same and so r.chdir does not really make sense anymore?

[gh-timokau]

Yes that is right. I wasn't aware that there was a difference before. I think sharing a working directory is actually less confusing. You're right that r.chdir should be removed if we don't want to restore the old behaviour.

[mmezzarobba]

downgrading priority to minor in view of the last comments

[mkoeppe]

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[mkoeppe]

Setting a new milestone for this ticket based on a cursory review.