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

k4run: allow addition of additional parameters from steering files #134

Merged
merged 8 commits into from
Aug 10, 2023

Conversation

Zehvogel
Copy link
Contributor

@Zehvogel Zehvogel commented Aug 4, 2023

Supersedes #131

With this change you can add a parameter from a steering file by simply doing something like

from k4FWCore.parseArgs import parser
parser.add_argument("-f", "--foo", type=int, help="hello world")
my_opts = parser.parse_known_args()
print(my_opts[0].foo)

this is achieved by wrapping the python argparse.ArgumentParser into a singleton to share it with the steering file without any global variable magic. I also got rid of the usage of the ApplicationMgr global variable in k4run.

BEGINRELEASENOTES

  • k4run: allow addition of additional parameters from steering files

ENDRELEASENOTES

@Zehvogel Zehvogel changed the title Argparse2 k4run: allow addition of additional parameters from steering files Aug 4, 2023
@Zehvogel
Copy link
Contributor Author

Zehvogel commented Aug 8, 2023

@hegner here the PR I mentioned

Copy link
Contributor

@hegner hegner left a comment

Choose a reason for hiding this comment

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

Thanks. Looks like a rather annoying bug you fixed. Solution is nice.

@hegner
Copy link
Contributor

hegner commented Aug 8, 2023

And it seems the failing builds are unrelated

@hegner hegner merged commit a58b287 into key4hep:main Aug 10, 2023
2 of 5 checks passed
@jmcarcell
Copy link
Contributor

jmcarcell commented Aug 18, 2023

This PR shouldn't have been merged. This is the list of PRs with fixes for what this PR broke:

If I'm not wrong the restructuring of the file loading wasn't needed for this PR which was a relatively simple change so it should have been in a different PR.

@giovannimarchiori
Copy link
Contributor

giovannimarchiori commented Aug 28, 2023

FYI, this PR broke running fccrun with the following macro: https://github.com/BrieucF/LAr_scripts/blob/main/FCCSW_ecal/runTopoAndSlidingWindowAndCaloSim.py

fccrun runTopoAndSlidingWindowAndCaloSim.py 
/home/gmarchio/work/fcc-main/FCCDetectors/
Traceback (most recent call last):
  File "/cvmfs/sw-nightlies.hsf.org/key4hep/releases/2023-08-28/x86_64-almalinux9-gcc11.3.1-opt/k4fwcore/948460e49c28c33a342434334119b89969569113=develop-rstv2h/bin/k4run", line 127, in <module>
    load_file(file)
  File "/cvmfs/sw-nightlies.hsf.org/key4hep/releases/2023-08-28/x86_64-almalinux9-gcc11.3.1-opt/k4fwcore/948460e49c28c33a342434334119b89969569113=develop-rstv2h/bin/k4run", line 26, in load_file
    exec(file.read())
  File "<string>", line 79, in <module>
  File "<string>", line 79, in <listcomp>
NameError: name 'path_to_detector' is not defined

Code works fine with Aug10 nightly but fails with Aug11 one

@Zehvogel
Copy link
Contributor Author

I can reproduce that bug, but it looks like a weird quirk of the combination of python list comprehensions and exec() see e.g. https://stackoverflow.com/questions/45132645/list-comprehension-in-exec-with-empty-locals-nameerror

Previously we passed globals() to exec so this didn't show up but that also leaked every local variable into the k4run execution.

I will look for a fix tomorrow.

@Zehvogel
Copy link
Contributor Author

Zehvogel commented Aug 29, 2023

@giovannimarchiori can you check if the change to k4run in #140 fixes your problem?

@giovannimarchiori
Copy link
Contributor

Hi @Zehvogel
the initial problem I reported is fixed by #140 but then a new one appears

/home/gmarchio/work/fcc-main/FCCDetectors/
Traceback (most recent call last):
  File "/home/gmarchio/work/fcc-main/k4FWCore/install/bin/k4run", line 131, in <module>
    add_arguments(parser, ApplicationMgr())
  File "/home/gmarchio/work/fcc-main/k4FWCore/install/bin/k4run", line 65, in add_arguments
    parser.add_argument( f"--{propName}", f"--{propNameReversed}", type=proptype, help=props[prop][1],
  File "/cvmfs/sw-nightlies.hsf.org/key4hep/releases/2023-06-24/x86_64-almalinux9-gcc11.3.1-opt/python/3.10.10-4lh2y5/lib/python3.10/argparse.py", line 1441, in add_argument
    return self._add_action(action)
  File "/cvmfs/sw-nightlies.hsf.org/key4hep/releases/2023-06-24/x86_64-almalinux9-gcc11.3.1-opt/python/3.10.10-4lh2y5/lib/python3.10/argparse.py", line 1807, in _add_action
    self._optionals._add_action(action)
  File "/cvmfs/sw-nightlies.hsf.org/key4hep/releases/2023-06-24/x86_64-almalinux9-gcc11.3.1-opt/python/3.10.10-4lh2y5/lib/python3.10/argparse.py", line 1643, in _add_action
    action = super(_ArgumentGroup, self)._add_action(action)
  File "/cvmfs/sw-nightlies.hsf.org/key4hep/releases/2023-06-24/x86_64-almalinux9-gcc11.3.1-opt/python/3.10.10-4lh2y5/lib/python3.10/argparse.py", line 1455, in _add_action
    self._check_conflict(action)
  File "/cvmfs/sw-nightlies.hsf.org/key4hep/releases/2023-06-24/x86_64-almalinux9-gcc11.3.1-opt/python/3.10.10-4lh2y5/lib/python3.10/argparse.py", line 1592, in _check_conflict
    conflict_handler(action, confl_optionals)
  File "/cvmfs/sw-nightlies.hsf.org/key4hep/releases/2023-06-24/x86_64-almalinux9-gcc11.3.1-opt/python/3.10.10-4lh2y5/lib/python3.10/argparse.py", line 1601, in _handle_conflict_error
    raise ArgumentError(action, message % conflict_string)
argparse.ArgumentError: argument --SimG4Svc.SimG4FullSimActions.OutputLevel/--OutputLevel.SimG4Svc.SimG4FullSimActions: conflicting option strings: --SimG4Svc.SimG4FullSimActions.OutputLevel, --OutputLevel.SimG4Svc.SimG4FullSimActions

@Zehvogel
Copy link
Contributor Author

@giovannimarchiori It looks like your file has a property of the same name multiple times and we try to add the argument to set it more than once. I think this was implicitly filtered out in the old implementation by dict.fromkeys().

Can you provide us with complete reproduction instructions in #141? In particular how to set up the environment? For me your options file already fails with

Traceback (most recent call last):
  File "/home/lreichen/work/k4FWCore/./k4FWCore/scripts/k4run", line 133, in <module>
    load_file(file)
  File "/home/lreichen/work/k4FWCore/./k4FWCore/scripts/k4run", line 26, in load_file
    exec(file.read(), {})
  File "<string>", line 384, in <module>
  File "/cvmfs/sw-nightlies.hsf.org/key4hep/releases/2023-06-24/x86_64-almalinux9-gcc11.3.1-opt/python/3.10.10-4lh2y5/lib/python3.10/os.py", line 680, in __getitem__
    raise KeyError(key) from None
KeyError: 'FCCBASEDIR'

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.

4 participants