epic icon indicating copy to clipboard operation
epic copied to clipboard

fix: set materials-map string constant with path to materials map

Open wdconinc opened this issue 2 years ago • 7 comments

Briefly, what does this PR introduce?

This works with https://github.com/eic/EICrecon/pull/826 to set the materials-map string constant in the geometry so we can avoid having to assume that calibrations/materials-map.cbor is where we want the materials map to be searched for. The latter approach leads to race conditions when the same geometry directory is used by multiple running processes which keep updating the calibrations/materials-map.cbor file with what they want, only to read it later when it might have changed again already.

What kind of change does this PR introduce?

  • [x] Bug fix (issue: possible cause for craterlake eicweb reconstruction benchmark failures)
  • [ ] New feature (issue #__)
  • [ ] Documentation update
  • [ ] Other: __

Please check if this PR fulfills the following:

  • [ ] Tests for the changes have been added
  • [ ] Documentation has been added / updated
  • [x] Changes have been communicated to collaborators: @ShujieL @bschmookler

Does this PR introduce breaking changes? What changes might users need to make to their code?

No.

Does this PR change default behavior?

Yes. The materials map will be written to a different file name. An updated EICrecon will be required to pick it up automatically. Alternatively, you can explicitly pass the option e.g. -Pacts:MaterialMap=calibrations/materials-map.cbor.

wdconinc avatar Aug 03 '23 23:08 wdconinc

I have a couple questions:

  1. Can we add the value from the plugin itself, using https://github.com/AIDASoft/DD4hep/blob/ad3c96d6ff95dca60d510fe78f0cffa25068c81c/DDCore/include/DD4hep/DetectorImp.h#L344 The downside is that it possibly creates a precedent of a constant not defined in xml.
  2. Instead of name suffixes like -arches would it make more sense to use content-addressed filenames (include file hash in the name)?

veprbl avatar Aug 03 '23 23:08 veprbl

Can we add the value from the plugin itself

I think you already hint at the reason: it puts more stuff inside the plugins, which I'd rather not do because it hides it from where users would naturally go and look for this info.

wdconinc avatar Aug 04 '23 18:08 wdconinc

@wdconinc Is this ready to merge?

rahmans1 avatar Feb 07 '24 03:02 rahmans1

Nah, let's keep it for next month, if at all.

wdconinc avatar Feb 07 '24 04:02 wdconinc

It doesn't like something here... https://eicweb.phy.anl.gov/EIC/benchmarks/reconstruction_benchmarks/-/jobs/2503876

wdconinc avatar Feb 13 '24 04:02 wdconinc

  01:44:29    epic.getDete   INFO      Adding material from /home/runner/work/epic/epic/scripts/material_map/calibrations/materials-map.cbor
  Traceback (most recent call last):
    File "/home/runner/work/epic/epic/scripts/material_map/material_validation_epic.py", line 46, in <module>
      detector, trackingGeometry, decorators = epic.getDetector(args.xmlFile, args.matFile)
    File "/home/runner/work/epic/epic/scripts/material_map/epic.py", line 33, in getDetector
      matDeco = acts.IMaterialDecorator.fromFile(
    File "/usr/local/python/acts/__init__.py", line 61, in _decoratorFromFile
      return ActsPythonBindings.JsonMaterialDecorator(
  RuntimeError: Unable to open input JSON material file: calibrations/materials-map.cbor
  ./action_payload.sh: Error on line 12: ./run_material_map_validation.sh
  /home/runner/work/_actions/eic/run-cvmfs-osg-eic-shell/main/setup-eic-shell.sh: Error on line 28: $THIS/run-linux.sh

veprbl avatar Apr 16 '24 02:04 veprbl

Right, that's a place where I've hard-coded it ... sigh.

wdconinc avatar Apr 16 '24 02:04 wdconinc