DETECTOR is not deprecated, but serves different purpose than DETECTOR_CONFIG

[wdconinc]

In https://github.com/eic/EICrecon/blob/main/src/services/geometry/dd4hep/JDD4hep_service.cc#L66 eicrecon generates the warning

[WARN] DETECTOR environment variable is deprecated. Use DETECTOR_CONFIG instead

even when DETECTOR_CONFIG is defined (that's only checked later).

This is somewhat misleading since DETECTOR_CONFIG is what defines the detector configuration (e.g. epic_arches, epic_tracking_only) under a specific DETECTOR description (e.g. epic). The DETECTOR_CONFIG starts with DETECTOR.

This means that DETECTOR is not deprecated. It just isn't the variable that should be used to pass the configuration to use.

[DraTeots]

Is there any place where those collaboration global variables are described?

Because previously in our documentation was written that one sets "DETECTOR" environment variable such as epic_arches . And then it was said that instead of DETECTOR environment variable DETECTOR_CONFIG should be used. In order to make smooth transition for scripts I made a deprecation warning for using DETECTOR env as epic_arches and suggested that DETECTOR_CONFIG should be used. In terms of EICrecon it is a deprecation.

[DraTeots]

Lets kind of define what is the expected behavior

Now we have this in documentation

https://eic.github.io/EICrecon/#/design/common_flags_env?id=environment-variables

[DraTeots]

Is there any place where those collaboration global variables are described?

@wdconinc If you list it here it will also work

[DraTeots]

@wdconinc bump

[wdconinc]

You should not rely on hardcoded names of environment variables to pass the geometry but you should use the value of a parameter for a plugin (-Pdd4hep:geometry=/path/to/detector/geometry.xml). It should not matter to EICrecon what convention is used in the epic repository. Any convention in that repository is a local convention and will not extend to other geometry repositories. Relying on that convention is reintroducing a dependency that we worked to remove early on.

[DraTeots]

That, I believe, needs to be passed to @faustus123 as he had a very strong argument, that in general -Pdd4hep:geometry=/path/to/detector/geometry.xml shouldn't be passed EVERY eicrecon run especially as this value most of the time is not changing. It is bad from users perspective, if every time, they call eicrecon, they should type in -Pdd4hep:geometry=/path/to/detector/geometry.xml.

At the same time, -Pdd4hep:geometry has priority. So if it is given, it is used independently on what is there in environment.

As of my opinion on this, I was strongly on your side (@wdconinc) in the beginning, but after speaking with David and thinking changed my mind. The decision, to use environment variables to set default values as this would cover 99% cases - is balanced between users conveniece and public interface goodness.

But you are correct, that Any convention in that repository is a local convention and will not extend to other geometry repositories so we could think of changing it to something Eicrecon or domain specific like:

EICRECON_DD4HEP_DETECTOR_PATH
EICRECON_DD4HEP_DETECTOR_CONFIG

Or if we will split EICrecon to some generic parts + EIC part, that could be this generic part thing. Better to have a convention ready then for such things.

P.S. What I concern more of, is that I believe having WHATEVER_DETECTOR_CONFIG without extension (.xml) is confusing. It would be better to have a full path only.

[faustus123]

It looks like the printed warning with the misleading "deprecated" statement was removed in PR#293 11 days ago. I'm going to close this as an issue for now and suggest any further discussion be brought up in another forum since it is much broader than this one piece of code.

[wdconinc]

In 0.3.4 currently in eic-shell there are still references to DETECTOR alongside DETECTOR_CONFIG:

ERROR: file: /opt/local/share/epic/epic.xml does not exist!
Check that your DETECTOR and DETECTOR_CONFIG environment variables are set correctly.