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

Discussion: How to best deal with the different IO backends #131

Closed
tmadlener opened this issue Sep 2, 2020 · 3 comments
Closed

Discussion: How to best deal with the different IO backends #131

tmadlener opened this issue Sep 2, 2020 · 3 comments

Comments

@tmadlener
Copy link
Collaborator

tmadlener commented Sep 2, 2020

NOTE: this issue started out as the one that is described in this first description here, but later changed to a slightly different but related topic below

Currently the options for generating the datamodel used for the tests are:

options :
# should getters / setters be prefixed with get / set?
getSyntax: False
# should POD members be exposed with getters/setters in classes that have them as members?
exposePODMembers: True
includeSubfolder: True

Consequently, these are also the only things that are currently really covered by the tests. In #130 a fourth option, createSIOHandlers, will be added, making the number of possible combinations even larger. While we probably do not need to exhaustively test all of the combinations, at least having every option tested properly once would be a nice thing to have, I think.

The question now is how to most easily achieve this. Two options that came to my mind are the following (probably by far not the only ones):

  1. Introduce several datamodel.yaml files with different values for the different options. Since some of the options are potentially not possible with the current definition (e.g. exposePODMembers would lead to name clashes currently) this could be necessary in any case.
  2. Expose the options as command line flags to the generator so that they can be changed when actually generating the c++ files. This would allow to reuse the same datamodel.yaml for multiple tests and simply calling the generator with different flags to generate the corresponding c++ files for multiple different options. However, this would introduce functionality which is only here for easier testing and not something that is needed for the users of podio.

In both cases the tests folder would probably need a bit of restructuring, to keep the different cases nicely separated. Additionally, the getSyntax option would also require to rewrite some of the tests while using a different syntax to access the members. On the other hand that would be an option that could be considered to not need exhaustive testing apart form successfully compiling it.

At least for #130 the easiest solution would be to have option 2 available, since we could then pass the corresponding option from cmake to the code generator and would automatically have the corresponding files generated. Currently, we can either generate with createSIOHandlers: False and break the newly introduced tests or we can generate with True, which will break the build if SIO_HANDLERS=OFF in cmake.

Another, somewhat related, question is whether we want to enable building the tests and the test datamodel by default. Only building the things that are in the end installed takes probably less than 20 % of the build time, while building the tests, and especially the datamodel takes rather long.

@tmadlener tmadlener mentioned this issue Sep 15, 2020
11 tasks
@andresailer
Copy link
Member

continuing from #130 (comment)

Regarding the handling of the generation of different IO systems, I have started some kind of discussion in that direction in #131. Maybe instead of setting environment variables, we could pass command line arguments to the generator from cmake? Or would that defeat the purpose of exporting PODIO_IO_HANDLERS? I, personally, find explicitly passing command line arguments a bit easier to debug and to follow the flow of the whole thing.

command line arguments (or a single one taking a list) can also be used, I was going the lazy way of environment variables to pass the options, to avoid changes in the argument parser of the podio_class_generator.

I think one still has to declare in podioConfig.cmake which handlers are available. Of course this could be done by looking at a list of targets and creating the list from there. But this is probably more maintenance then adding to a single list whenever a new handler is created.
I think using the list of handlers in the PODIO_GENERATE_DATAMODEL cmake function is also most straight forward, and then you can just keep the list and put it in the podioConfig.cmake.

For exporting the dependency on SIO, it would be ideal if SIO already exports targets, right?

I don't think it is mandatory, but it would be easier. For any purpose targets would be preferable.

@tmadlener tmadlener changed the title Discussion: How to best deal with the different generator options in tests and CI Discussion: How to best deal with the different IO backends Sep 17, 2020
@tmadlener
Copy link
Collaborator Author

Since some of the discussion in #130 shifted towards how to most easily incorporate different IO backends and also how to best propagate these to downstream targets, I have changed the topic of this issue slightly.

I think passing the information which backend specific code to generate to podio_class_generator.py is basically a matter of taste (i.e. environment variables or command line arguments), but this information should not be present in the yaml file that defines the datamodel, to avoid having conflicts here.

Ideally PODIO_GENERATE_DATAMODEL (or another cmake function) generates the c++ source code and already builds the core datamodel library as well as additional backend specific libraries. I am not sure if something like this is easily possible with an indefinite number of possible backends, that are built depending on the cmake configuration.

For the core podio functionality, i.e. a backend specific reader and writer, I think we need either a cmake function that builds the corresponding library or we place them into separate sub folders and only include them depending on the cmake configuration. Maybe both? Additionally this probably requires abstract interfaces that the readers and writers have to fulfill to easily have a core EventStore (or something similar) that can talk to all possible backends.

Any thoughts on this?

@tmadlener
Copy link
Collaborator Author

This has been been addressed by the cmake macros introduced with #130.

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

No branches or pull requests

2 participants