Skip to content

Do not set properties twice for ApplicationMgr and MessageSvc in the job options #329

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jmcarcell
Copy link
Member

@jmcarcell jmcarcell commented Jun 12, 2025

It seems the options service has access to the properties of ApplicationMgr and MessageSvc and that it has had it for a long time. I checked and it's the same properties (podio-dump -c configuration_metadata -d <file>). For the NTupleSvc, since no one seems to be using it (does it even work for us?) I propose to simply remove them.

BEGINRELEASENOTES

  • Do not write properties twice for ApplicationMgr and MessageSvc. Do not write any properties for the NTupleSvc
  • Construct strings in place instead of using std::stringstream
  • Remove an extra ; at the end of each pair of property and value, they are anyway each different strings

ENDRELEASENOTES

Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test that checks that these config options are written properly?

@jmcarcell
Copy link
Member Author

I can add a test but it's not trivial. First, we don't have a defined format, and the current one is a mix of things. I'm not sure about checking exactly the properties that exist now since this can change for example from Gaudi and we have to fix it.

For the format currently everything goes between quotes but this creates cases where the value is a string that is also in quotes: b'TimelineSvc.TimelineFile = "\'timeline.csv\''"\n. We could get rid of the \n character after every parameter since they are already different strings, but then podio-dump won't display them in different lines even though when reading from podio one gets a list.

@tmadlener
Copy link
Contributor

OK. I think at some point we should add a check to see whether we can run the config that we store.

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.

2 participants