DAFNI Workflow Enhancements

[f-allian]

Enhancements to the DAFNI workflow that enables better usability and reproducibility. These include:

• Removed variables.json as an argument. The variables can now be stored as metadata attributes in the dot file produced by the user.
• Developing a local test suite to ensure the entrypoint works as expected.
• Turned main_dafni.py into a module, which improves the modularity and testability of the entrypoint in the test suite.
• CI/CD workflows have also been updated.

[github-actions]

🦙 MegaLinter status: ✅ SUCCESS

Descriptor	Linter	Files	Fixed	Errors	Elapsed time
✅ PYTHON	black	36		0	0.96s
✅ PYTHON	pylint	36		0	5.63s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.01%. Comparing base (18d0356) to head (05a85fd).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #299   +/-   ##
=======================================
  Coverage   97.01%   97.01%           
=======================================
  Files          29       29           
  Lines        1842     1842           
=======================================
  Hits         1787     1787           
  Misses         55       55           

---

Continue to review full report in Codecov by Sentry.

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

[jmafoster1]

Would it make sense to document the metadata attributes here?

[f-allian]

@jmafoster1 I was thinking of documenting this on the DAFNI platform itself, rather than in this README. Both?

[jmafoster1]

I think both is good. I've been using the main_dafni.py as a main entrypoint for other things to save me having to write a separate run script each time, so it makes sense to document it for users who might not have access to the DAFNI platform.

[f-allian]

@jmafoster1 Happy to do both, however, this raises a separate issue that needs resolving in the future. The /dafni directory, including the entrypoint, should technically only be used by DAFNI users. We wouldn't want to encourage CTF users to use main_dafni.py as the entrypoint to the CTF (as there are obvious limitations to this script). But I'll put a pin in this for now.

[jmafoster1]

In that case, would it make sense to have the dafni stuff be a separate repo? We may have had this discussion before, but if we only want the dafni stuff used on that platform, it probably doesn't make sense to distribute it with the CTF. Alternatively, a longer term goal could be to broaden the applicability of the entrypoint, but that doesn't invalidate this PR, so we can do that separately later.

[jmafoster1]

How are constraints expressed as node attributes? I thought constraints came in the causal tests JSON file?

[f-allian]

@jmafoster1 I didn't realise constraints are defined in the causal tests file. In that case, we'll need something separate to parse the JSON file for the constraints.

Do we have an example JSON file that I can use that uses constraints in the causal tests file? I can only find one in the Poisson example that defines constraints in the script but doesn't actually use it in the JSON file for some reason, which doesn't make sense to me.

[jmafoster1]

Yes, see #299 (comment)

The constraints in the script are an unused variable, so I guess they weren't transfered to the JSON file. We probably put them in at some point because they were mandatory and then made them optional but didn't bother to get rid of the variable. This stuff was written a LONG time ago before we had linting for PRs, so I guess it got missed.

[jmafoster1]

Expressing constraints in terms of individual variables is a real headache. I battled with this for about 6 months during my PhD. The problem is that constraints can relate variables, e.g. i1 > i2. Then you need to put the constraint i1 > i2 for node i1 and the inverse (i.e. i2 < i1) for node i2. (I guess you don't technically need to do this here, but then you have the question of which node you associate the constraint with.)

[jmafoster1]

When I did the CARLA case study I encoded queries on a per-test basis using an attribute called "query", which is a silly name for it here but made sense at the time because you can then just do df = df.query(test["query"]) and it'll "just work" if the user supplies the query in the correct format. I propose we do a similar thing here.

[jmafoster1]

Since we just return the inputs and outputs as separate lists and then join them up later, would it make sense to just return one list of variables defined as

variables = [
        Input(node, eval(eval(attributes["datatype"]))) if attributes["typestring"] == input else
        Output(node, eval(eval(attributes["datatype"]))) if attributes["typestring"] == output
        for node, attributes in causal_dag.graph.nodes(data=True)
]

It's not really a problem either way, but it's just a bit more elegant

[f-allian]

@jmafoster1 I think it's less clear here what's being calculated and returned as a single line. You'd then have to define inputs = variables[0] which is less informative than the current method.

[jmafoster1]

My point was that we never use inputs or outputs in isolation. We build them separately and return them separately, but it looks like the only thing we do with them is append them anyway, so I was wondering whether it made sense to treat them separately at all, but if you think it's more elegant the way you have it then we can leave it as is. It's not like people will be dealing with millions of inputs/outputs.