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

Silence when SIO is missing and exit if the podio dictionary is not found #498

Closed
wants to merge 4 commits into from

Conversation

jmcarcell
Copy link
Member

@jmcarcell jmcarcell commented Oct 3, 2023

BEGINRELEASENOTES

  • Silence when SIO is missing so that the messages are not shown every time a datamodel is generated when a podio installation already exists
  • Simplify __init__.py, remove an unneeded import ROOT

ENDRELEASENOTES

Copy link
Collaborator

@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.

This would break in completely pristine environments, where there is no previous podio installed, I think. We are only building the libpodioDict.so, but we still need some parts of the python bindings to run the code generation (which is done at CMake stage, so no chance that we can build the dictionary before hand).

@jmcarcell
Copy link
Member Author

jmcarcell commented Oct 31, 2023

PR updated. I'm a bit upset that there isn't a way of loading a library silently so that we wouldn't need to check first and then load (that will do the same check again).

Edit: I think I'm happy now with the changes. There is one possible simplification for loading because we can use

ROOT.gInterpreter.Load("lib.so")

and it will not print any output whether it is successful or not, and that can be checked with the returned value. However, for the cases when there is a mismatch between ROOT versions ROOT.gInterpreter may not say anything but ROOT.gSystem does

@tmadlener
Copy link
Collaborator

However, for the cases when there is a mismatch between ROOT versions ROOT.gInterpreter may not say anything but ROOT.gSystem does

By "not say anything" you mean that gInterpreter doesn't give us a different return value in case there is a version mismatch, or simply that it doesn't print a warning message?

Comment on lines +12 to +13
except ImportError:
print('Unable to load podio, make sure that libpodio.so is in LD_LIBRARY_PATH')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have to re-raise this here, or does this already propagate up after the log output?

@hegner hegner self-requested a review November 1, 2023 09:46
@jmcarcell
Copy link
Member Author

However, for the cases when there is a mismatch between ROOT versions ROOT.gInterpreter may not say anything but ROOT.gSystem does

By "not say anything" you mean that gInterpreter doesn't give us a different return value in case there is a version mismatch, or simply that it doesn't print a warning message?

I'm not sure about this. I just tested building podio with 6.28.04 and then loading the podio library with a new build with master and none of the two ways complain but I remember seeing some message when the ROOT versions were not the same... I initially thought that maybe it doesn't say anything because gSystem.Load calls gInterpreter.Load with an extra check about the versions: https://root.cern.ch/doc/master/TSystem_8cxx_source.html#l01929 and https://root.cern.ch/doc/master/TSystem_8cxx_source.html#l01933. When the library doesn't exist gSystem.Load complains with a message while gInterpreter.Load just returns -1. So maybe it would be fine to use gInterpreter.Load to do the lookup once only

python/podio/__init__.py Outdated Show resolved Hide resolved
@tmadlener
Copy link
Collaborator

Picked up by #511 and being merged as part of that.

@tmadlener tmadlener closed this Nov 8, 2023
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