Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Branch for functional testing design #1280

Closed
wants to merge 1 commit into from

Conversation

MichaelClerx
Copy link
Member

Something like this?

@codecov
Copy link

codecov bot commented Feb 9, 2021

Codecov Report

Merging #1280 (b46d95b) into master (ae46a5a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1280   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           84        84           
  Lines         8823      8823           
=========================================
  Hits          8823      8823           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae46a5a...b46d95b. Read the comment docs.

@fcooper8472
Copy link
Member

Not sure on a few points:

  1. What would import functional_testing do? Where is the functional_testing module, what does it do, and why would it be needed? I had imagined a method that returns some stuff.
  2. The results definitely shouldn't be stored in PINTS. The functional tests, I think, should just return something like {'kld':1.23,} and the external functional testing runs each method 10 times and puts the results somewhere.
  3. Why is there a ft_haario_bardenet_mcmc_banana.py containing a class, and a ft_haario_bardenet_mcmc_banana_2.py containing just methods?

Sorry @MichaelClerx I think I might be not on the same wavelength about this!

@MichaelClerx
Copy link
Member Author

MichaelClerx commented Feb 9, 2021

Hi @fcooper8472 ! I was imaging it would work just like the unit tests frameworks in Python. For example unittest and pytest.

In unittest, tests are grouped in a class which extends some base class, hence the import. An example of what this would look like is in the first file (without a number)
Unittest example: https://github.com/pints-team/pints/blob/master/pints/tests/test_dual_averaging.py

In pytest, tests are just methods, so the import isn't needed. An example of this is in the _2 file.
Pytest example: https://github.com/ModellingWebLab/cellmlmanip/blob/master/tests/test_parser.py

In both examples, the github action would:

  • install functional_testing (a python library we write)
  • check out and install pints, including some submodule where the results are stored
  • (call some functional_testing method to) find the tests, using a pointer to a directory, then a template for file names (ft_*.py), then some python introspection
  • (call some functional_testing method to) run the tests (10 times), and update the results

When developing a new method a dev would

  • write a new test class, that goes in the pints repository, in the functional-tests directory
  • use functional_testing, installed locally, to run the test. In this case no storage happens, results are simply printed to screen. (Possibly graphs are shown which are not generated when called in automation mode?)

Finally, unlike the unit tests, I imagine the functional tests won't be part of the pints package, as there is no benefit for the users of having them all on their system. (As opposed to the unit tests, which let them provide us with debugging info!)

@fcooper8472
Copy link
Member

Slightly frustrating, because I don't think that's what we discussed, and I think this new design certainly warrants discussion.

I'm probably not understanding something, but why do we need a whole functional testing module just to find and run tests?

What's wrong with:

from pints.functional import test_haario_bardenet_mcmc_banana

results = []
for i in range(10):
  results.append(test_haario_bardenet_mcmc_banana())

Just completely cuts out the complexity.

Then the functional testing repo itself just imports everything from pints.functional, runs everything 10 times, deals with putting the results somewhere sensible and updates the website.

Why would a PINTS user want a tool just to run a function?

@MichaelClerx
Copy link
Member Author

Slightly frustrating, because I don't think that's what we discussed, and I think this new design certainly warrants discussion.

I'm probably not understanding something, but why do we need a whole functional testing module just to find and run tests?

What's wrong with:

from pints.functional import test_haario_bardenet_mcmc_banana

results = []
for i in range(10):
  results.append(test_haario_bardenet_mcmc_banana())

Just completely cuts out the complexity.

Then the functional testing repo itself just imports everything from pints.functional, runs everything 10 times, deals with putting the results somewhere sensible and updates the website.

Why would a PINTS user want a tool just to run a function?

I thought that was exactly what we discussed :D

In the minimal case you could run without having the tool installed, sure. But otherwise it becomes a dev tool just like the ones we already use (e.g. flake8).

All I'm saying is that we have PINTS know how to use functional_testing, instead of having functional_testing know how to use PINTS, just like you suggested? So functional testing becomes some library with tools for generating the website etc., but its PINTS that makes calls to it, so that we can trigger tests on PINTS repo updates etc?

@ben18785
Copy link
Collaborator

ben18785 commented Feb 9, 2021

Thanks @MichaelClerx Yes, I was sort of thinking it would work as Fergus envisioned (perhaps even running the loop)...

@MichaelClerx
Copy link
Member Author

MichaelClerx commented Feb 9, 2021

I'm really puzzled now!

This PR is a draft / proposal for the code that goes in PINTS, that describes a test in terms that functional testing can understand.

What Fergus suggested a while ago (pints-team/change-point-testing#38) was to

  • "Move the actual functional tests to the main PINTS repo" - this is what my 2 examples are. They are tests. Using either a class layout (like unittest does) or a method layout (like pytest does). They are competing proposals. Note that in the pytest version a test is simply a method inside a python file.
  • "Functional testing repo would just contain code for running those tests, helping to separate code that uses PINTS from the infrastructure that runs the functional test." - this is what my functional_testing module/tool does.
  • "Functional testing needs to be run from the PINTS repo, or there is no sane way to run it on just push to master on PINTS. This would mean writing a functional testing GitHub workflow on PINTS that just checks out functional-testing, with a token if necessary to push changes" - this is why pints uses the functional_testing module, instead of the other way around.
from pints.functional import test_haario_bardenet_mcmc_banana

results = []
for i in range(10):
  results.append(test_haario_bardenet_mcmc_banana())

👆🏻👆🏻👆🏻 This, is an example of what would go inside functional_testing (so not this PR, which is a draft for a test, stored in pints). My two draft proposals are for the module test_haario_bardenet_mcmc_banana

(In my proposal the first line would be a clever bit of re-usable code that scans a directory, exactly the way unit tests currently work, but we can add that later once we think about making it re-usable.)

The only slight difference I can see is this:

  • "Let's stop over-thinking how we store results. Instead of having a database that sits somewhere that we have interact with, let's just keep a data directory in functional testing". I was thinking if we want to - ultimately - have functional_testing be reusable then it would make more sense to store the results in pints. I suggested a submodule to stop cluttering pints main.

@fcooper8472
Copy link
Member

"Functional testing needs to be run from the PINTS repo, or there is no sane way to run it on just push to master on PINTS. This would mean writing a functional testing GitHub workflow on PINTS that just checks out functional-testing, with a token if necessary to push changes" - this is why pints uses the functional_testing module, instead of the other way around.

I stand by my original statement, but your suggestion that therefore pints needs to use functional_testing does not make sense to me.

To clarify, the part I am not understanding is why import functional_testing would ever be a line in PINTS code. Functional testing should be a (possibly private) repo that deals with running the tests and doing something with the results, and is definitely NOT code that someone using PINTS should be seeing or interacting with.

Hence

from pints.functional import test_haario_bardenet_mcmc_banana

results = []
for i in range(10):
  results.append(test_haario_bardenet_mcmc_banana())

would be user code for interacting with the functional test, which I think should just be an importable function in a directory of pints.

@MichaelClerx
Copy link
Member Author

To clarify, the part I am not understanding is why import functional_testing would ever be a line in PINTS code.

Fair enough. The only reason you'd do that is to implement an interface defined in the functional testing code.
This is how unit tests currently work, they import unittest and then define a class that implements a unittest interface.
But when tests are run in CI it's unittest that calls the pints method.

An alternative popular testing framework is pytest, in which tests don't implement an interface, so you don't need the import
Unless you want to do clever things via decorators, which you then do import pytest for.
Again, when tests are run in CI it's pytest that calls the pints method.
Offline users also have the option to do something like pytest tests/this_particular_test.py if they want to use extended pytest features, but there's nothing stopping them from just running it directly.

The pytest setup is most similar to what we discussed (just a method). But as we use unittest for unit tests already I included an example of both.

Functional testing should be a (possibly private) repo that deals with running the tests and doing something with the results, and is definitely NOT code that someone using PINTS should be seeing or interacting with.

100% agreed. That's why this PR has the functional tests outside of the main pints module. pip users don't get the functional tests. Only devs who checkout the repo do.

But the github action that calls all this stuff lives in the pints repo. Right? So in a stable production world the pints repo would contain tests (like the current pints/tests/*), config files (like .coveragerc, .flake8, etc.), and a github action that uses the functional testing module to run the tests defined in pints.

which I think should just be an importable function in a directory of pints.

that's exactly what the pytest version of this PR is

@fcooper8472
Copy link
Member

Could we just have a new directory alongside pints/tests called pints/functional_tests with an __init__.py and a bunch of tests, so that the functional tests ARE actually part of PINTS?

That way if a user wants to run them, maybe because one day we want to tweak the hyperparameters, they can just import pints.functional_tests as ft or something, and call the methods directly?

The GitHub action would do the following steps:

  • checkout pints (as normal)
  • checkout functional_testing
  • run functional_testing/do_everything.py --pints=/path/to/pints

@MichaelClerx
Copy link
Member Author

Yeah I'm OK with that 👍🏻

I was thinking the directory with the functional tests would double up as the results directory, so maybe be a submodule, but that's all dependent on future decisions on how we store and such 😄

@fcooper8472
Copy link
Member

Yeah, I think the easiest/quickest/dirtiest way to get started will be to dump some csv files in the functional testing repo. But I've been looking at using something like Azure Cosmos-DB which has a free tier that seems to be WELL within our usage requirements.

@MichaelClerx
Copy link
Member Author

See #1295

@MichaelClerx MichaelClerx deleted the functional-testing-design branch March 23, 2021 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants