Balance CI testing responsiveness and depth

[Silvanoc]

Current CI testing approach is quite heavy (time- and resource-wise):

1. The whole test suite is run per push on PRs.
2. Each run takes 10-15min.
3. It is run on 7 different combinations of platforms and versions.

I would like therefore to propose a 2 stages merge process:

1. Lightweight and quick tests (<1min possible?) are run on a single platform and version per push.
2. The approval of a PR by one maintainer triggers pre-merge full-blown tests (all platforms and versions, involving external validation -like GraphQL validator-, services -like a Kroki server-,...).

The proposal has the obvious advantages of:

1. Shorter feedback round-trips than now with less pressure on the infrastructure.
2. Testing "weight" does not become a burden for all developers and the infrastructure and does not hinder the project from introducing more and better "heavy" tests.

---

More complex and powerful 3 stage alternative:

In this alternative an additional stage is added:

1. Very lightweight and quick tests are run on a single platform and version per push as long as the PR is in draft status. Ideally, if it can be somehow correlated, only tests very relevant according the changes (for example, only tests under tests/tests_generators for changes in the directory linkml/generators).
2. More heavy tests are run per push if the PR is in ready status.
3. The approval of a PR in ready status of one maintainer triggers pre-merge tests that are full-blown (all platforms and versions, involving external validation -like GraphQL validator-, services -like a Kroki server-,...)

This proposal has following advantages:

1. Motivate developers to stay in draft status until stabilization, providing them shorter feedback cycles and discharging reviewers from their review effort.
2. Better feedback quality in ready than on draft, but still keeping the feedback round-trip shorter than now. A developer can roll-back to draft if shorter feedback desired.
3. Testing "weight" does not become a burden for all developers and the infrastructure and does not hinder the project from introducing more and better "heavy" tests.

[sneakers-the-rat]

when i started working on linkml the tests took an hour to run on my machine, so we've come a long way here :)

running tests locally without --with-slow (eg. when one just does python -m pytest) takes 4m12s on my machine, and when run in parallel with pytest-xdist (python -m pytest -n auto) they take 1m05s.

I'm trying to close up my laptop and head home for the night so am not gonna profile rn, but here are some ways we could improve test perf:

library improvements:

• induced_slot is still by far the slowest method, so perf enhancements there
• remove hbreader and replace with something that uses requests so that we can use VCR and not do any network requests.
• ctrl+f things that claim to be deprecated in comments, mark them as deprecated, and then remove them and their tests (eg markdowngen)

test cleanup:

• remove duplicate tests - there are a lot of tests for the same thing, notably in the test_issues directory
• remove tests without assertions - probably a holdover from another time when tests were run directly rather than via pytest?
• use cheaper schemas that directly target the object of the unit test - the kitchen_sink schema is often used when it's unnecessarily costly to generate and it's not cached. eg. see the sole projectgen test.
• remove ShexGen from CORE_FRAMEWORKS in compliance tests, because it is currently always skipped (again not with pytest.skip)
• finish marking tests that use the metamodel as slow, there are some lingerers still out there

test infra improvements:

• don't double-canonicalize RDF graphs when comparing: currently when doing snapshot tests we load the snapshot, canonicalize it, canonicalize the generated graph, and compare for equality. this takes a loooong time. If we instead canonicalized when serializing the snapshot, we could just do a single canonicalized serialization and test for string equality.
• eagerly skip compliance tests when ValidationBehavior is Incomplete, Not Applicable, or False Positive: the actual compliance test part of the test is skipped with a warning (rather than pytest.skip so tests appear to pass), but time is still taken to generate the output for that framework. any time savings in the compliance tests is worth a lot since there are like 7500 of them. Rather than logging behavior at the end of check_data, we could just make a fixture for registering framework behavior for a compliance test that runs during test teardown (after the yield) rather than needing to go through the entire check_data function, which is why i assume we aren't already skipping these.

When i remove shexgen from compliance and eagerly skip (by regexing a skip call before every check_data call, which doesn't skip validated_schema in most cases which generates the test output which usually takes the most time) I go from 4m12s -> 3m50s for free. skipping the remaining metamodel tests that aren't marked slow brings us down to 3m30. When I change the scope of the kitchen_sink_path fixture to module and load the schema (rather than returning a path so each test loads it anew) we get down to 3:00, but that doesn't really count because it causes some tests to fail in generators that mutate the loaded schema.

anyway i think we actually have a decent amount of floor to squeeze out of the tests before we start dumping em <3

---

That said I think a multi-tiered testing situation is a great idea. In linkml-runtime we just test that package until an approval is received, and then we run upstream linkml tests against that versions. It has proven (in my opinion) to be a bit of a pain in the ass because to run the upstream tests again you have to get another approving review, but i really like the idea of omitting marked slow tests when a PR is in draft stage.

Another possibility is to split off slow tests into another workflow/job, so the not slow tests run on every push, and then the slow tests run against first/last supported python version when it comes out of draft status.

Ideally, if it can be somehow correlated, only tests very relevant according the changes (for example, only tests under tests/tests_generators for changes in the directory linkml/generators).

I have started adding marks for the different generators, they're not comprehensive yet but if they were then we could use PR labels for this - only run tests for a given generator when it's one of the labeled generators (and then run all tests in whatever we decide as the "final phase" when all tests are run)

[Silvanoc]

IMO the different performance activities that you mention are probably meaningful and probably tackle my concern. But being a bit egoistic here, I find the multi-tiered testing approach less costly to implement and addresses my concerns 🙂

[sneakers-the-rat]

Like i said im all for it, go hogwild with @pytest.mark.slow as far as im concerned (not like it's my call anyway ;)

[vincentkelleher]

I like the multi layered approach @Silvanoc 👍
Feedback loop speed is critical to make developers' lives easier and to motivate people to contribute. I won't mention the advantage of being able to tackle production issues and produce fixes quicker as there is no production service running for LinkML.

@sneakers-the-rat as usual your knowledge of this project's code base just blows my mind 🤯 You clearly know what we need to optimize.
I totally agree with your idea of making more specific lightweight integration test schemas instead of reusing humongous kitchen sinks 👍

I will emphasize one point that's critical and should be the first to be addressed : the Kroki.io error blocking the CI verification steps.
This issue doesn't need an improvement but a fix, it's truly a blocker in my opinion. As I stated previously, we should have a LinkML Kroki server instance running somewhere, I don't mind helping in the setup and maintenance of this instance as I already deployed one before, I just don't want the whole CI to rely on hardware that I manage or own as we don't have specific place to host this instance 😇

[Silvanoc]

I like the multi layered approach @Silvanoc 👍 Feedback loop speed is critical to make developers' lives easier and to motivate people to contribute.

That is my main motivation for this discussion. On my first contributions I could not get the testing environment properly to run and therefore was relying on the CI tests. But since it took so long, I was extremely demotivated.

Now that I can have quick feedback locally using pytest-xdist (thanks @sneakers-the-rat for that) and ignoring slow tests, I am much more productive. Still waiting for almost 1/4h to get the final confirmation is APITA.

@sneakers-the-rat as usual your knowledge of this project's code base just blows my mind 🤯 You clearly know what we need to optimize.

I fully agree on this 🙂

I will emphasize one point that's critical and should be the first to be addressed : the Kroki.io error blocking the CI verification steps. This issue doesn't need an improvement but a fix, it's truly a blocker in my opinion. As I stated previously, we should have a LinkML Kroki server instance running somewhere, I don't mind helping in the setup and maintenance of this instance as I already deployed one before, I just don't want the whole CI to rely on hardware that I manage or own as we don't have specific place to host this instance 😇

As stated somewhere else, my proposal is to check that the make the right Kroki REST calls based on snapshots on local systems. And to use Docker on CI or, optional, developer systems with Docker available.

We still have the issue, though, that Windows test VMs do not appear to provide Docker, so they need some external service 😦

[sneakers-the-rat]

Now that I can have quick feedback locally using #2347 (thanks @sneakers-the-rat for that) and ignoring slow tests

does pytest not skip these automatically for you? my goal last spring was to try and get test times below 5m from stock default settings and if i have failed there then there's more work to do!

also definitely advise adding marks to any tests that are missing them as you go and using that. when i'm working i now just rely on doing pytest -m {generator name}gen over and over again and that usually completes in dozens of seconds depending on the generator. when i find a test failing in the complete tests that i missed in some limited -marked test set i make sure to add that mark. maybe we need more docs on this? i have been sorta sly with it and sneaking it into every PR lol.

As stated somewhere else, my proposal is to check that the make the right Kroki REST calls based on snapshots on local systems. And to use Docker on CI or, optional, developer systems with Docker available.

I totally agree with this - the thing that we're looking for here is VCR and the related pytest plugin, that basically is a standard way of recording the API response so from the program's point of view we just emit the API request, VCR catches it, returns the recorded response, and test proceeds as normal :)

[Silvanoc]

does pytest not skip these automatically for you? my goal last spring was to try and get test times below 5m from stock default settings and if i have failed there then there's more work to do!

Yes, they are automatically skipped. Sorry, my formulation was misleading.

also definitely advise adding marks to any tests that are missing them as you go and using that. when i'm working i now just rely on doing pytest -m {generator name}gen over and over again and that usually completes in dozens of seconds depending on the generator. when i find a test failing in the complete tests that i missed in some limited -marked test set i make sure to add that mark. maybe we need more docs on this? i have been sorta sly with it and sneaking it into every PR lol.

I don't use pytest marks as much as I should... I will make sure to use them more and to help improving the markers.

I totally agree with this - the thing that we're looking for here is VCR and the related pytest plugin, that basically is a standard way of recording the API response so from the program's point of view we just emit the API request, VCR catches it, returns the recorded response, and test proceeds as normal :)

No matter I much I would like to see VCR in use, I do not really think that we really need it here. I mean the only call we do to the Kroki server is a GET call without any special headers to receive a file. If we split the logic "create request URL" and "contact the server", then we can test the URL creation easily and leave the "contact the server" part for CI to contact a real Kroki server. The VCR approach does not provide much over "URL creation testing" and does not test against a real server (which API might change over time) either. So IMO is does not bring enough to be worth its introduction.

Additionally, I have given it a try, and it would need fixing the automatic cassette name generation to work for us:

OSError: [Errno 63] File name too long: '/Users/mch01250/repositories/github/linkml/linkml/tests/test_generators/cassettes/test_serialize_selected[Person-\\nclass "Person" [[{A person, living or dead}]] {\\n    {field} id : string  \\n    {field} name : string  \\n    {field} age_in_years : integer  \\n    {field} species_name : string  \\n    {field} stomach_count : integer  \\n    {field} is_living : LifeStatusEnum  \\n    {field} aliases : string  [0..*]\\n}\\n].yaml'

🤷

[Silvanoc]

One stage that we are not considering yet and would be probably meaningful to introduce is a pre-release stage. In this stage all full-blown tests would need to pass before a new release is created. Probably a release process that automatically blocks releases until all tests are green would be meaningful. AFAIK releases are as of now a somehow manual process.

[sneakers-the-rat]

firmly @cmungall territory and i think unifying the metamodel build process -> into linkml-runtime+linkml testing scenarios here would be a very satisfying thing that maybe takes some of the load off <3