Add Gaudi Functional C++ Class Generator#372
Conversation
This script generates Gaudi Functional C++ classes with appropriate structure and boilerplate code based on user-defined specifications.
Updated the Gaudi Functional C++ Class Generator to support k4FWCore framework, improved input/output specifications, and added new functionalities for property parsing and class generation.
Added command_line parameter to generate_class function and updated its usage in main.
|
@BrieucF - please, check. Thanks! |
|
Without any tests, inevitably this will not work in the future if something changes and we won't be able to know. |
Thanks for looking into it. A very good point! Shall I add the generation and compilation for it to CI tests? Thanks |
|
@tmadlener weren't you also working on something like this? :) |
|
Yes, sort of. I have a semi-working thing for a static website. But that also doesn't have the tests it should have. In the end, I don't care too much which version lands as long as one does. |
tmadlener
left a comment
There was a problem hiding this comment.
Thanks a lot for this. I think the general idea of the script goes into the exact right direction, i.e. we want something that generates boilerplate for something that the user wants. I didn't look in extreme detail, but I see a few general improvements (also partially highlighted in the inline comments):
- Currently the user needs to know which functional type they want. But this is entirely specified by the number of inputs and outputs, so I would just determine that from there.
- Generally the implementation could probably be improved by sprinkling a few classes into the whole thing for holding intermediate information instead of passing around tuples of strings of various lengths. This would probably also make it easier to simply do some of the parsing / processing up-front and then pass the information around instead of re-parsing it several times.
- The current implementation does not handle the possibility of variable length inputs / outputs (I think). This is only possible in
k4FWCoreFunctional algorithms though.
I think this is borderline complex enough to warrant the use of a template engine to handle all the string formatting. It introduces some overhead, and would require writing some templates, but it would simplify the python script parts by quite potentially.
As already mentioned we definitely need tests that ensure that the outputs compile. That would probably be the first thing I do, because once that is in place refactoring and extending the implementation can be done with some guard rails.
| if len(out_types) == 0: | ||
| return "void()" |
There was a problem hiding this comment.
This case can never happen as this is already ruled out in the parsing of the arguments. Also, I think even if this were to happen, it doesn't compile because a Producer needs at least one output.
| # Remove Collection suffix and namespace | ||
| clean_name = typ.split('::')[-1].replace('Collection', '') | ||
| loc = clean_name | ||
| lines.append(f'KeyValues("{loc}", {{"{loc}"}})') |
There was a problem hiding this comment.
I think from the current implementation there is no way we will ever need the KeyValues, but we always want the KeyValue. Unless the current version already handles inputs/outputs of type std::vector<const XYZCollection*> in which case there should be a differentiation here and only those get a KeyValues (of the appropriate length), single inputs should get a KeyValue.
| if 'edm4hep::' in typ: | ||
| # Extract all collection types (handle nested templates) | ||
| # Match patterns like edm4hep::MCParticleCollection | ||
| pattern = r'edm4hep::(\w+Collection)' | ||
| matches = re.findall(pattern, typ) |
There was a problem hiding this comment.
I don't think this differentiation is necessary at all. Algorithms will always take Collections as inputs/outputs. (I think trying to do anything different will not even compile).
| elif functional_type == 'transformer': | ||
| in_sig = ', '.join([f"const {t}&" for t in in_types]) if in_types else "" | ||
| if len(out_types) == 1: | ||
| return f"{out_types[0]}({in_sig})" | ||
| else: | ||
| out_sig = ', '.join(out_types) | ||
| return f"std::tuple<{out_sig}>({in_sig})" |
There was a problem hiding this comment.
I don't think this compiles for multiple outputs. It's either a Transformer (single output) or a MultiTransformer multiple outputs so depending on that we also need to generate a different class here.
tmadlener
left a comment
There was a problem hiding this comment.
As mentioned in the meeting today already it would be nice to have tests for this. Given that you also generate a minimal cmake script this should be fairly straight forward, essentially for various options (I would say each type of algorithm at least once) one would
- Run the script (probably inside a "sandbox" directory) to generate source code and cmake
- Run cmake
- Run build
The main problem with that is that we would have to hook that into the existing test harness in the CMakeLists.txt if we just want to run it as part of the "standard" CI. An alternative option would be a custom github actions workflow where we could run this without that harness. (Opinions @jmcarcell?).
In any case I think a first step would to just write some bash scripts for each option with the three steps above and then we can decide later where / how we hook them up to CI).
Some other general comments:
gaudi_gen.pyis probably not descriptive enough. In the end people would like to use this from within a Key4hep environment and I think something likegeneratFunctional.pyorgenerateAlgorithm.py(or even without the.pysuffix) would make that a bit more explicit- This needs to be installed for it to be truly useful, otherwise people will have to clone k4FWCore and get the script from there.
For that something similar to what we do for k4run:
k4FWCore/k4FWCore/CMakeLists.txt
Line 20 in dd8d891
In this case it would probably have to be something like
gaudi_install(SCRIPTS helpers)or alternatively via an explicit call to cmake install.
|
I tried this and there are several options that are not explained, then I tried them and they seem not to do anything. Like
I wonder at which point, in a repo that already has algorithms, it's just easier to copy and paste an existing algorithm than get the command right to run the script. If there are many different times one has to copy and paste all of them with all the |
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
Add documentation for gaudi_gen.py script, including usage, requirements, arguments, and examples.
Refactor gaudi_gen.py to address reviewers comments, improve code organization and readability. Changes include removing unused imports, updating function signatures, and enhancing argument help descriptions.
Updated shebang to use 'uv run' for script execution and added metadata for dependencies.
Clarified requirements and usage instructions for the script, specifying the need for Jinja2 only in certain contexts and providing detailed invocation methods.
BEGINRELEASENOTES
ENDRELEASENOTES
Gaudi Functional C++ Class Generator
This script generates Gaudi Functional C++ classes with appropriate structure and boilerplate code based on user-defined specifications. It supports both Gaudi::Functional and k4FWCore frameworks.
Features
Consumer,Producer,Transformer,FilterPredicatepodio::UserDataCollection<float>structby default, Gaudi usesclassby default-hto see all functional types and examplesInput Format
For inputs/outputs:
TypeName:LocationNamepodio::UserDataCollection<float>)For properties:
Type:Name:DefaultValue:DescriptionExamples
k4FWCore Producer (reproduces ExampleFunctionalProducerMultiple)
k4FWCore Consumer
python gaudi_gen.py MyConsumer consumer \ -i 'edm4hep::MCParticleCollection:MCParticles'k4FWCore Transformer
Gaudi Framework Examples
Command-Line Options