WIP: ACTSFW-104 Introduce python based geometry tests
This adds tooling to execute our extrapolation examples and uses RegExes to parse the geometry construction details from it's stdout. This output is then compared against a reference stored in acceptance_tests/ref/*json. This can serve as a regression test, if, for example, the geometry has a different binning than before, this will show up.
In addition, I implemented a relatively dumb parser for the DD4hep XML format in python, which allows me to derive "expected" values for dimensions, binnings, number of layers etc. directly from the input file. These results can then be checked for in the aforementioned geometry building output. A failure here indicates a change in one of the geometry creation algorithms.
I introduced a CI job which uses the build output and runs pytest, which in turn executes:
- the generic detector example
- the DD4hep setups: TestTracker1, TestTracker2 and IBLSimple
- the FCChhBaseline If any of these produces either regressions or doesn't match whats expected based on the XML, the job fails.
If the geometry building output changes, the RegExes might have to be adjusted accordingly. The parsing should be robust enough to ignore any changes to other output.
I hope that this gives us (more precisely: me) more confidence when making changes that can have effects in edge cases that have not been anticipated, and thus are not covered in the other (unit) tests. I think the variety of setups now included in these tests should cover a lot of edge cases.
Before this can get merged, ACTS MR !369 needs to be merged into core.
Thoughts?
Merge request reports
Activity
I feel somewhat uneasy about tests that try to parse a program's standard output. The Gaudi framework uses this approach everywhere, and it does not work so well:
- Any minor formatting change in the logged output can break the test.
- Any reordering of the output (including those induced by multithreading) breaks the test as well.
- Minor differences in the output (e.g. random ROOT warnings about failing to connect to the network when opening a disk file -- don't ask) turn into test errors.
- Adjusting to cross-machine variability (e.g. different line endings, system component versions, timestamps) easily becomes time-consuming.
- As human-readable output tends to be rather parsing-hostile, the stdout parsing regexps easily end up causing more maintenance effort than the associated test itself.
If possible, I would advocate an alternative approach in which any program output which should be analyzed by a unit test or integration test is sent to a dedicated file, which is designed for machine parsing and held to higher reproducibility standards than the standard output normally is.
Edited by Hadrien Benjamin Grasland117 117 // mapped 118 118 // hand over modules to ACTS 119 119 Acts::ActsExtension::Config layConfig; 120 layConfig.isLayer = true; 121 layConfig.axes = "YxZ"; 122 layConfig.materialBins1 = 100; 123 layConfig.materialBins2 = 100; 124 layConfig.layerMaterialPosition = Acts::LayerMaterialPos::outer; 125 Acts::ActsExtension* detlayer = new Acts::ActsExtension(layConfig); 120 layConfig.isLayer = true; 121 layConfig.axes = "YxZ"; 122 /// @todo re-enable material mapping 123 // layConfig.materialBins1 = 100; 124 // layConfig.materialBins2 = 100; 125 // layConfig.layerMaterialPosition = Acts::LayerMaterialPos::outer; -
Should this be taken care of before merging? (there are multiple other occurences in the MR)
Edited by Hadrien Benjamin Grasland 
This is a result of the convolution with @jhrdinka's work which I based this branch on. Before merging I would clean this up, maybe cherry pick. This was supposed to be a basis of discussion as of now.
-
I agree that parsing the output is not the most robust way to gather testing information. To achieve the type of structured output you mentioned, we would need to include output facilities in the core geometry building. That is certainly possible, but at the very least is a substantial architectural task, if we want this to be more than just "shoved in there" somehow.
One way I could imagine would be a configurable "info stream" which can be routed to a file and gets a (for instance) JSON dump of the created layers.
At this point; I think the output parsing is still viable in this case is that it allows us to run the examples without any kind of test setup, exactly as you would in case you just ran it by hand. The type of test in this MR doesn't replace a real unit test (which I also wrote for most of the code covered by this) but instead replaces or automates "running it and looking at the output". This is kind of tedious to do by hand for the 5 detectors I included here.
I don't think it's harmful to start doing the tests this way, and replacing them with something more elegant down the road. At this point, I prefer having this type of test to no test at all.
-
@hgraslan I could go ahead and try to include a type of "test-output" setup to the LayerCreator. Would you be opposed to just introducing that kind of mechanism at this one location for now? Does this have to be designed in a way that it can fit other components?
-
So you would argue that a setup like this would also have to be thread safe? Synchronization of writing this type of output seems like something that you would want to be able to disable for production execution, which means that there would have to be some kind of "debug mode".
Then again, that's exactly what I was kind of happy about avoiding with the implementation in this MR, namely being able to "just execute" the examples that we have anyway, without a dedicated test setup.
Edited by Paul Gessinger-Befurt -
I agree with keeping the experiment restricted to the LayerCreator at this point in time. Since the geometry dump interface will only be used internally for testing and debugging purposes, I think we can do this by marking said interface as unstable and subjected to change in the future.
This allows us to start experimenting with this quickly without needing to roll out a complete geometry testing solution right from the start. Once we have more experience with this testing methodology and want to extend it to the rest of the geometry building, we will be free to break all the temporary interfaces and design a more general solution, leveraging the experience that was acquired meanwhile.
Regarding thread safety requirements, the general stance of ACTS on this is that only code which runs in the event loop needs to be thread safe, and code which runs sequentially at initialization and finalization time does not need to be thread safe. Since geometry building does not happen inside of the event loop (I think?) and is not internally parallel, we probably do not need it to be thread safe. I was only pointing out threads as one more source of stdout variability that I've observed in Gaudi, but I don't think they are relevant in this specific case.
Edited by Hadrien Benjamin Grasland added 2 commits
added 1 commit
- 57f744b4 - move generic detector example to variant_data json output
added 1 commit
- 5025be7c - migrate geometry acceptance tests to new variant data