Clean up a fractured repository

[Anaphory]

I have finally taken the steps to take @xrotwang's and my changes that have accumulated over the time of about a year, and cast them into something that passes all tests (I hope I haven't accidentally removed any regression tests) and has a slightly more sensible structure in the tree prior module.

Last November, I messed with data readers. In December, I changed some tree prior behaviour. Now, I have cleaned up the tree prior behaviour and cleaned up everything to work with the internal API changes that @xrotwang worked on around last August.

This is a MASSIVE change – I hope not in terms of the API, though. I'm sure something has slipped by. But it seemed to be better than letting all changes go to waste and trying to think again from scratch what to do.

[Anaphory]

The wiser thing would obviously have been for me to review #248 first, so that the changes included there could make it into master and the burden of change on this PR is smaller.

[Anaphory]

Concerning the failing tests that need fixing:

• [ ] 3.4 is affected by #253
• [ ] 3.5 has attr issues, which I thought were solved by #215
• [ ] 3.7 and 3.8 need pytest_cov, which is not available (I think _cov is a relatively new fork??)

[xrotwang]

Regarding the attr issues with py3.5: AFAIR this is basically an issue with travis. Upgrading attr (and possibly pytest) will fix these.

[xrotwang]

I guess the best way forward given the current status would be

• merge this
• add more tests (and possibly a bit of refactoring)
• release as next major version (basically admitting that we don't really know what the backwards compat story is, here)

[Anaphory]

As per #233, I have a suggestion to explicitly break backward compatibility by throwing out the Coalescent prior, so I would be happy to do all of that in one go.

[xrotwang]

@Anaphory trying to run the tests with py 3.5 gives:

(beastling) forkel@shh.mpg.de@dlt4803010l:~/venvs/beastling/BEASTling$ pytest
ImportError while loading conftest '/home/forkel/venvs/beastling/BEASTling/tests/conftest.py'.
tests/conftest.py:7: in <module>
    from beastling.configuration import Configuration
beastling/configuration.py:20: in <module>
    import beastling.treepriors.base as treepriors
E     File "/home/forkel/venvs/beastling/BEASTling/beastling/treepriors/base.py", line 10
E       tree_id: str = "Tree.t:beastlingTree"
E              ^
E   SyntaxError: invalid syntax

I don't know how widely you introduced type annotations into the codebase, but I think we should hold back with such a change until we are clear about which python versions to support.

[xrotwang]

Ok, after fixing two instances of type annotations, I get the tests to run - with 5 fails in beastrun_tests. Could these be due to differing BEAST installations? Missing BEAST modules? It would seem good, to also fix the BEAST installation details for reproducible testing. How could we do this?

[Anaphory]

I don't know how widely you introduced type annotations into the codebase, but I think we should hold back with such a change until we are clear about which python versions to support.

True! I keep forgetting how new the features are that I have really started to appreciate these days.

[Anaphory]

Ok, after fixing two instances of type annotations, I get the tests to run - with 5 fails in beastrun_tests. Could these be due to differing BEAST installations? Missing BEAST modules? It would seem good, to also fix the BEAST installation details for reproducible testing. How could we do this?

We could provide a copy of ~/.beast/2.6/ somewhere, which is the place BEAST installs packages to.

The failed tests are likely due to you not having installed (1) SA, which is necessary for the FBD prior and easy to install from CBAN/Beauti; and (2) statereconstruction, which I still don't know how to get into CBAN, and even if you had a version of it, it looks like I messed with the package name at some point, changing it from something to something else. I'd still like to make that package accessible and rely on it, so we can provide ancestral state reconstruction in several nodes.

[xrotwang]

@Anaphory let tox remind you! I think running tox locally - at least for major changes - is really good practice.

[xrotwang]

Hm. A copy of BEAST seems too heavy. Isn't there a way to specify packages for beauti? I remember some sort of XML which is read by beauti. Maybe we could use a portion of this as equivalent to requirements.txt for BEAST?

[xrotwang]

So, SA installed, still 5 fails. Looks like all 5 pull in ancestral_state_reconstruction.conf - so I guess it's this statereconstruction package. Maybe we should mark the 5 tests as xfail?

[Anaphory]

Looks like all 5 pull in ancestral_state_reconstruction.conf - so I guess it's this statereconstruction package. Maybe we should mark the 5 tests as xfail? That sounds like a good plan for now.

[xrotwang]

@Anaphory so here's what's needed to make tests pass on 3.5:

diff --git a/beastling/treepriors/base.py b/beastling/treepriors/base.py
index 4597a98..89219b0 100644
--- a/beastling/treepriors/base.py
+++ b/beastling/treepriors/base.py
@@ -7,12 +7,12 @@ from beastling.util import xml

 class TreePrior (ABC):
-    tree_id: str = "Tree.t:beastlingTree"
+    tree_id = "Tree.t:beastlingTree"

     def __init__(self):
         # Define loggable states, together with a log level
         # TODO: This is not yet used at all.
-        self.loggables: t.Dict[str, int] = set()
+        self.loggables = set()

     def add_state_nodes(self, beastxml):
         """
diff --git a/tests/beastrun_tests.py b/tests/beastrun_tests.py
index 18ca084..542fc12 100644
--- a/tests/beastrun_tests.py
+++ b/tests/beastrun_tests.py
@@ -162,8 +162,6 @@ skip = [
                 lambda dir: dir.joinpath("beastling_test.log").exists()),
     ]
 )
-
-
 def test_beastrun(configs, assertion, config_factory, tmppath):
     """Turn each BEASTling config file in tests/configs into a
     BEAST.xml, and feed it to BEAST, testing for a zero return
@@ -171,6 +169,9 @@ def test_beastrun(configs, assertion, config_factory, tmppath):
     if configs in skip:
         return

+    if 'ancestral_state_reconstruction' in configs:
+        pytest.xfail('ASR depends on BEAST functionality which is not installed')
+
     with warnings.catch_warnings(record=True) as w:
         warnings.simplefilter("always")

[xrotwang]

oh, and possibly pytest>=5.4 in setup.py to make travis happy.

[lmaurits]

Obviously I'm late to this party, but I want to get this big clean up finished this week and release a new BEASTling version - it's recently come to my attention that, not too long ago, the most recent release became uninstallable via pip because it has dependencies on particular versions of clldutils and/or pycldf which can no longer be simultaneously satisfied, which is not good. This problem was already solved in the repo long ago, we just need to actually get it out.

[lmaurits]

Closed by accident!

[lmaurits]

Do we think it's best to merge this PR as it is and then tackle Python version compatibility issues, or get everything working in this branch?

Regarding version support, I generally like to be pretty conservative about this (not just regarding Python itself, but also all third party dependencies as well), but obviously the maintenance burden increases the more seriously one commits to this so I'm happy to be pragmatic. It seems like 3.6 is the oldest version still actively maintained by PSF, so I won't object to dropping 3.4 support. If keeping 3.5 working is not a major chore I'd prefer to do it, but it depends on what's involved.

[lmaurits]

I just ran Travis again for this PR and Python 3.5 and it looks like it is now dying on type annotation stuff instead of attr stuff, so it looks like @xrotwang was right about this and it just required an update on Travis' side (it's pretty horrific this kind of thing can happen, I think I retroactively regret letting something like attr in).

I need to look into it, but a priori I am hugely skeptical of massive language changes like type annotations. To be honest, the last time I looked at some really cutting edge Python code I didn't even recognise it as Python and though it was some new-fangled Python-inspired language.

[xrotwang]

@lmaurits @Anaphory The problem with py3.5 is not type annotations in attrs code, but in beastling code - see https://github.com/lmaurits/BEASTling/pull/250#issuecomment-648682598 So, while depending on attrs was a bit of a pain in the past, I think it isn't anymore.

[xrotwang]

Ah, sorry. I misread "type annotation stuff instead of attr stuff" as "... inside of ...".

[xrotwang]

I also share the reservations regarding type annotations. As far as I'm concerned, py3.5 compat would be more important, than type annotations - in particular just one or two of these sprinkled randomy through the code base, rather than systematically using type annotation (and tooling to exploit these).

[lmaurits]

It looks like Travis is happy now with all of 3.6, 3.7 and 3.8. Since I'm happy to dump 3.4, that means the only sticking point left is 3.5. If that's simply a matter of removing a small number of type annotations that have crept in, I think we should do that and retain 3.5 compatibility for now.

[xrotwang]

Agree. I think it is just the two lines shown in https://github.com/lmaurits/BEASTling/pull/250#issuecomment-648832140 I can't add commits to this PR, since it's from @Anaphory 's fork, though.

[xrotwang]

Oh, btw. we might want to move away from travis, since it's shutting down the free tier - as far as I know. Moving to GitHub Actions is pretty simple, see e.g. https://github.com/cldf/pycldf/commit/1e82dd376198032cc5a0280167713d3df6b59533

[lmaurits]

Oh, btw. we might want to move away from travis, since it's shutting down the free tier - as far as I know.

Wow, really? Their homepage boasts

Testing your open source projects will always be free! Seriously. Always.

Anyway, regardless of the case, I would be happy to move to GitHub Actions. Not out of any fondness for GitHub, but we're already using them anyway and the fewer third party services we are reliant on the better.

[xrotwang]

I guess the

Please be aware travis-ci.org will be shutting down in several weeks, with all accounts migrating to travis-ci.com.

is viewed by many as the next step towards charging for the service.

[lmaurits]

I now have GitHub a workflow in place for testing Python 3.5 - 3.9, but the tests fail due to the unavailability of a beast executable (for me, the tests pass locally using tox with Python 3.8). With Travis we were, IIRC, simply not doing the beastrun tests there, and relying upon local testing. I don't remember how we actually did this, or how to duplicate it with GitHub...

[lmaurits]

Ah, okay, we check in tests/beastrun_tests.py for an environment variable of TRAVIS and if it's there don't do the test.

According to https://docs.github.com/en/free-pro-team@latest/actions/reference/environment-variables it looks like we could s/TRAVIS/CI/g and we should get the same behaviour...

[lmaurits]

And the same for tests/cli_test.py...

[xrotwang]

Sounds ok. It might be somewhat more transparent to use pytest markers, and then call pytest -m "not beast" in the GH actions yaml.

[lmaurits]

That sounds good, although the environment variable change did work and master now passes on 3.5 - 3.9 with GitHub testing.

Any objection to me merging this now (which should break the 3.5 test) and then fixing the type annotations as you've outlined above?

[xrotwang]

Go ahead!

[lmaurits]

Oh, yikes, this PR was against develop. Am I misremembering, or was the plan to do away with that branch?

[lmaurits]

It was, see #230, so let's do that at the same time as this big 2.0.0 release.

[xrotwang]

Yes, I think the idea was to drop the "gitflow" development model.

[lmaurits]

Okay, I've merged develop into master. Can't actually delete develop yet as there's an open PR still.

I've deleted .travis.yml, uninstalled the Travis webhook on GH, and updated the README badge to use the new GH Workflow. I think we're Travis-free, now. master is passing all GH tests on Python 3.5 - 3.9.

Our codecov integration seems broken? Not sure why yet...

[xrotwang]

For one repository, I also experienced problems with codecov, so I simply cut it out, i.e. removed

    - name: "Convert coverage"
      run: "python -m coverage xml"
    - name: "Upload coverage to Codecov"
      uses: "codecov/codecov-action@v1"
      with:
        fail_ci_if_error: true

I think for BEASTling we could do the same, since coverage measurements are more important locally, when run with beastrun.