ASR tests fail

[xrotwang]

I can't get the ASR tests to pass on my machine. Do they require an additional BEAST package? I have BEASTlab, BEAST_CLASSIC and MM, but nothing else installed.

[Anaphory]

Oh, my AncestralStatesLogger (in statereconstruction) may have crept back into develop. I'll check this afternoon and revert it back to the standard BEASTLabs AncestralStateLogger (which can log only a single state and does it badly) or maybe check Babel's AncestralStateLogger2 to see whether that is a useful replacement.

[xrotwang]

@Anaphory doesn't look like it:

~/venvs/beastling/BEASTling$ grep -r AncestralStatesLogger .

vs.

$ grep -r AncestralStateLogger .
./docs/advanced.rst:  AncestralStateLogger class does not permit more than one MRCA to be
./beastling/beastxml.py:        """Add a logger referencing all AncestralStateLogger likelihoods in the tree."""
./beastling/models/basemodel.py:                    distribution.attrib["spec"] = "AncestralStateLogger"

[xrotwang]

It's possible, though, that the tests expect your logger's functionality, and therefore fail.

[xrotwang]

That's what the test config looks like - seems to try reconstruction of multiple states:

[admin]
log_trees = True
[model model]
reconstruct = f0,f3,f4

[xrotwang]

More investigation turned up that also another test fails: The one using

"admin", "covarion_multistate", "log_fine_probs"

and BEAST reports the error:

Could not find object associated with idref featureLikelihood:model:f0

[Anaphory]

@lmaurits Looks like we have another ID issue :( Where do they all come from?

[lmaurits]

At this point I am less interested in where they all come from than I am in why on Earth our tests aren't catching them. This morning I tried adding this method to BeastXml:

    def validate_ids(self):
        ids = []
        references = set()
        for e in self.beast.iter():
            for attrib, value in e.items():
                if attrib == "id":
                    ids.append(value)
                elif attrib == "idref":
                    references.add(value)
                elif value.startswith("@"):
                    references.add(value[1:])

        duplicate_ids = [i for i in ids if ids.count(i) > 1 ]
        if duplicate_ids:
            raise ValueError("Duplicate BEASTObject IDs found: " + ", ".join(duplicate_ids))

        ids = set(ids)
        bad_refs = [r for r in references if r not in ids]
        if bad_refs:
            raise ValueError("References to missing BEASTObject IDs found: " + ", ".join(bad_refs))

It threw many errors, but without this method in there my beastrun tests were all happy (except the ASR stuff which has been failing forever). This is extremely alarming and I don't know why it's happening.

[xrotwang]

I think the except subprocess.CalledProcessError as e catch isn' catching anything - at least not BEAST reporting an error. Maybe we need to check the return code of the process as well.

[lmaurits]

Yeah, actually, I think you're right. I just tried creating one of the XMLs which my id tester said should fail, and it did fail, and, argh, BEAST returned status code 0 anyway!

Waaaaaay back at the start of this whole thing I committed several changes to the BEAST core so that it would return non-zero codes when failing, to make this testing possible. Maybe the recent BEAST 2.6 release for some mind-boggling reason reverted these changes???

[xrotwang]

Yes, it's weird. I'll check whether stderr being non-empty is a useful criterion. But given that BEAST reports the errors on stdout, I'm not sure.

[lmaurits]

(passing observation while looking at BEAST code to try to figure this out - oh look, beast now takes a -sampleFromPrior argument. That's a nice idea! 😛)

[SimonGreenhill]

In case it helps, I've had luck validating BEAST XML with this:

        try:
            with open(os.devnull, 'w') as null:
                rv = subprocess.check_call(
                    [beast2, '-validate', self.filename],
                    stdout=null, stderr=subprocess.STDOUT
                )
        except subprocess.CalledProcessError:
            raise ValueError("beast couldn't validate the XML")

(and thanks, sampleFromPrior was my idea :)

[xrotwang]

Ok, I guess we'll have to switch to @SimonGreenhill 's method, since stderr is always used by BEAST for debug output like this

[java, -Xms8g, -Xms256m, -Djava.library.path=/usr/local/lib:/usr/java/packages/lib/amd64:/usr/lib/x86_64-linux-gnu/jni:/lib/x86_64-linux-gnu:/usr/lib/x86_64-linux-gnu:/usr/lib/jni:/lib:/usr/lib, -cp, ::/home/forkel/.beast/2.5/MM/lib/MM.addon.jar:/home/forkel/.beast/2.5/BEAST_CLASSIC/lib/mtj.jar:/home/forkel/.beast/2.5/BEAST_CLASSIC/lib/BEAST_CLASSIC.addon.jar:/home/forkel/.beast/2.5/BEASTLabs/lib/BEASTlabs.addon.jar:/home/forkel/.beast/2.5/BEAST/lib/beast.jar:/home/forkel/.beast/2.5/BEAST/lib/beast.src.jar, beast.app.beastapp.BeastMain, -java, -overwrite, /tmp/pytest-of-forkel@shh.mpg.de/pytest-40/test_beastrun_configs0_None_0/test]
java.lang.UNIXProcess$ProcessPipeInputStream@1b2c6ec2

[xrotwang]

Hm, but @SimonGreenhill 's check also relies on a non-zero return value from BEAST - which I don't seem to get for beast -validate either.

[xrotwang]

Ok, verified on BEAST 2.5.1:

>>> print(os.system('beast -validate _test.xml'))
...
0

when it prints Error detected about here: to stdout.

[lmaurits]

I've found one place where BeastMain will exit with status 0 after throwing a RunTimeException, but I'm not yet convinced that's what actually happens in this situation.

Fixing our apparently copious ID errors (which it turns out are easy to detect without even running BEAST) and making a bugfix release is a higher priority than coming up with a robust way to detect BEAST failures, IMHO. I'm going to commit that validate_ids method now, which will break All The Tests, then I'll try to fix the actual errors tonight.

[xrotwang]

Just upgraded to BEAST 2.6, and on validation error it returns 256.

[lmaurits]

Oh, that's a nice surprise! Maybe we can use that after all. Is this -validate option newish or have I just somehow not noticed it for all these years?

[xrotwang]

BEAST 2.6.0 also returns a non-zero value without the -validate flag. So I'm not sure why you didn't get test failures if you already ran this version.

[xrotwang]

Right now, all beastrun tests fail for me, due to idref errors. E.g. ('admin', 'mk', 'subsample'), with

Could not find object associated with idref data_model:f0

[xrotwang]

This is running from my fork, with tests ported to pytest.

[lmaurits]

I've fixed that data_model ID error, turned out to be very shallow.

Now validate_ids is giving me false alarms because it isn't <plate>-aware!

[xrotwang]

I'll take a shot at adding plate support

[lmaurits]

I already have something uncommitted that seems to be working, hold on...

[lmaurits]

I mean, of course, you are welcome to try in parallel if you want and may come up with something nicer than me.

[xrotwang]

I'm using a parent map to look up node parents for plate detection.

[lmaurits]

Ah, that is nicer. :) I was kind of shocked to find ET nodes don't have a parent attribute!

[xrotwang]

It is a bit shocking. But then, creating the parent map is a one-liner - so it's bearable.

[SimonGreenhill]

I'm pretty certain that the weirdness of ET is the reason why there are multiple XML libraries in python core as well as hundreds on PyPI

[xrotwang]

Ok, so now it seems as if something is wrong with log_fine_probs.conf. It introduces invalid idrefss in the logging section.

[lmaurits]

I just pushed a fix for that now. :)

[xrotwang]

Ok, merged. Will make PR with my validate_ids variant after confirming all tests pass.

[lmaurits]

Travis seems happy now!