Draft: Prevent output_file from being set when running on chained HLT

See #40 (comment 6728519)

mentioned in issue #40 (closed)

added RTA label

/ci-test

added ci-test-triggered label

Thanks for submitting a fix @lohenry. Is this definitely only a problem in run_chained_..., or should it be also in run_allen_in_moore... and run_moore_with_tuples? You can test this by running the HLT1 only and HLT2 only testing scripts and adding the extra options.output_file line as you did for hlt1 and HLT2.

• [2023-05-15 14:36] Validation started with lhcb-master-mr#7859
• [2023-05-19 08:03] Validation started with lhcb-master-mr#7910

mentioned in issue Moore#574 (closed)

approved this merge request

/ci-test

Started integration test build. Once done, check the results or the comparison to a reference build.

@sponce There are some new broken, but I could not relate them to the only change included in this MR.

Well the only change uses options.output_file and the error says : AttributeError: output_file. I think it's quite related indeed. It seems that these 3 tests do not have output_file and thus you cannot test it there.

Would

    if hasattr(options, "output_file"):
        if (options.output_file != ""):
            raise ValueError(
                "In run_chained_hlt_with_tuples: options.output_file should be set to '' as two writers would be configured."
            )

work?

I suppose so, but is that a solution or just avoiding the problem without understanding it. In other terms, do you get why output_file is not defined, and when this happens ?

Also you do not need 2 ifs, you can write

 if hasattr(options, "output_file") and options.output_file != "":

It makes some measure of sense to me that output_file could not be defined at all in cases where it is not needed: after all, this MR forces a similar behaviour.

Ok, but I would expect output_file to be present and empty. I know python allows to add/remove randomly members to a struct/class, but it still feels odd. options is supposed to be an object of type ApplicationOptions, where I would expect output_file to exist, even if empty. So it means options is not what we think, and this is worrying...

My fear is only that your fix is not taken into account in some cases where it's needed.

Hi @lohenry, any news about this fix?

options is a ConfigurableUser, so the "right" way is to

if options.getProp("output_file") != "":
    raise ...

assigned to @frodrigu

added 1 commit

• e495db8c - Test if options even has output file

Compare with previous version

mentioned in issue Moore#598 (closed)

mentioned in issue Moore#599 (closed)

assigned to @lohenry and unassigned @frodrigu

marked this merge request as draft

mentioned in issue Moore#623 (closed)

@lohenry shall we be pragmatic and close this MR? It seems we don't quite know what the underlying problem is or the proper fix, but I'd say that the edge case you've found has only come up this once in the roughly 4 years of HltEfficiencyChecker's existence. I also don't remember why someone would need an output_file in addition to HltEfficiencyChecker's usual ROOT output. In principle we should investigate further, but from an angle of prioritisation, it seems this one isn't super high-priority. What do you think?

Tend to agree. Maybe resolve as an Issue or not even?

I think unfortunately if we make it an issue then the issue will live forever and not be addressed unless it becomes higher-priority, so I think no issue. What do you think?

I feel it depends about how you want to manage the project, I have no strong feeling either way :D.

If it were a SciFi thing I would resolve as an issue as I sometimes review them and try to go for cheap fixes when I have the time, but on the other hand it does clutter the issue space.

I agree with this, but my problem here is that I personally really don't know how to debug this, and I'm kind-of the only person maintaining HltEfficiencyChecker. So I'd have to get an expert in to help me debug it/fix it, and I don't see it being high-enough priority for me to commit lots of time and an expert's time to fix it. This is why I don't think the issue would be addressed.

Although "forgetting" this issue is not ideal, I think it might be the pragmatic thing to do to not add clutter. FYI MooreAnalysis will be retired soon (HltEfficiencyChecker will move to DaVinci) so more reason not to add to the issue list.

I think if someone else comes across this issue then at that point we can consider elevating the importance of this, and perhaps opening an issue then.

Closing this now with the above conclusion @lohenry. If you feel strongly that this is not the right thing to do, please feel free to reopen and we can discuss it further.

closed