Project

General

Profile

Bug #4452

Misbehavior of ParameterSet::put(...) and insert(...) functions when key exists

Added by Christopher Green over 6 years ago. Updated about 5 years ago.

Status:
Closed
Priority:
Normal
Category:
Infrastructure
Target version:
Start date:
07/30/2013
Due date:
09/30/2013
% Done:

100%

Estimated time:
4.00 h
Spent time:
Occurs In:
Scope:
Internal
Experiment:
Mu2e
SSI Package:
fhicl-cpp
Duration: 63

Description

If the specified key for an insert or put exists, the operation will silently fail. It is also seriously non-trivial to ascertain whether a parameter exists (ParameterSet::get_keys() followed by std::vector<std::string>::find()).

At least, insert() and put() should replace any existing value. However, I believe it is also desirable to provide a bool has_key() method to allow the user to check for presence first.

History

#1 Updated by Marc Paterno over 6 years ago

The documentation for FHiCL, and common use, refers to the "keys" as names, so I would propose any new functions use the term 'name' rather than 'key'.

#2 Updated by Rob Kutschke over 6 years ago

Here is my two cents...

I like the addition of a "hasName" method.

I prefer that put/insert have an optional, argument that specifies what to do if the name already exists: the two options should be to replace the existing definition or throw an exception. I prefer that the default be to throw an exception. We could also consider a third option: to allow replacement if and only if the "type" of the old and new values match; that is, we can replace a sequence with another sequence but not with a table or an atomic value.

Perhaps it also makes sense to have a "replace" method that just forwards to "put" but which sets the optional argument to allow replacement.

#3 Updated by Christopher Green almost 6 years ago

  • Target version changed from 1.09.00 to 521

#4 Updated by Christopher Green over 5 years ago

  • Target version changed from 521 to 1.13.00

#5 Updated by Christopher Green about 5 years ago

  • Status changed from Accepted to Feedback
  • Assignee set to Christopher Green
  • % Done changed from 0 to 40

has_key() was added with fhicl-cpp:7ff98d3fdba1c9ea319eff5aa7fdd45c1eaef2cc.

We propose that insert() be taken private, because the ability to decode the encoded value relies on the correct construction of the boost::any, which cannot be left safely to the non-expert.

We further propose that put() throw if insert is not achieved, and that we provide the alternative function put_or_replace with the replace-if-existing behavior based on key only. Type checking is, unfortunately, not practical in the general case.

Does this sound reasonable?

#6 Updated by Rob Kutschke about 5 years ago

Please go ahead with the proposal.

I don't want to give up on the limited notion of type checking that we asked for but it's important to get the agreed upon functionality out the door. Should I open a new issue for the stuff we would still like - or is it so hard that it is unrealistic to ever expect it?

#7 Updated by Christopher Green about 5 years ago

The problem is that the only way to test whether a parameter is of a particular C++ type (or compatible with same) after encoding is to attempt the conversion and deal with the exception once thrown. It is generally not advisable to use exceptions as standard flow control as this (among other things) complicates debugging. There are several functions that aid one in distinguishing whether the key represents an atom, sequence or table, but once you have an atom determining its type requires an attempted any_cast(), because it is stored simply as a string.

#8 Updated by Rob Kutschke about 5 years ago

Sorry. I was not clear.

I was not requesting anything to do with C++ type. I only meant FHiCL type: if a FHiCL name is already defined as an atom, I would like an option to say that redefining it as a non-atom is an error. ( With the corner case that @nil is no type and can be redefined as anything ). I imagine that I only get this choice once per .fcl file - is that right?

#9 Updated by Christopher Green about 5 years ago

Ah, I understand now, sorry.

For the checking behavior you actually say you want, we would provide a function (say), put_or_replace_compatible() which would do the necessary checking and throw on an incompatible replace attempt. So, to summarize, the user-visible interface would be:
  • template< class T >
    void put (std::string const & key, T const & value);
    Throw on existing key.
  • template< class T >
    void put_or_replace (std::string const & key, T const & value);
    Generally succeed.
  • template< class T >
    void put_or_replace_compatible (std::string const & key, T const & value);
    Throw if replacing an item with a different major FHiCL type.

Does this seem reasonable?

#10 Updated by Rob Kutschke about 5 years ago

Yes. That's what I am looking for.

Is it also possible to make it configurable per policy so that the Mu2e executable chooses put_or_replace_compatible when the run time configuration is parsed?
I need to check with Andrei and a few others before I decide that this is what we really want. But I would like to know if it is a realistic request.

#11 Updated by Christopher Green about 5 years ago

Choosing the behavior in code is easy (whatever function the user decides to call); allowing the coherent behavior to be configured by the application at run time is hard, and not something we've thought about before. I'm loathe to do it the quick and dirty way (a global), but anything else will require thought and design. Can I suggest that we implement the suggestions I made earlier for the three different user-facing functions, and ask that you submit a different feature request for the run time configurability you desire?

#12 Updated by Rob Kutschke about 5 years ago

I like the proposed features - please get them in now.

As I remember the start of this discussion (maybe not captured well in the ticket) the goal was the catch a class of errors at FHiCL parsing time and for this feature to be available when the main art configuration file was parsed. But we can make this a separate ticket if needed.

#13 Updated by Christopher Green about 5 years ago

  • Status changed from Feedback to Assigned
  • Experiment CDF added
  • Experiment deleted (-)

#14 Updated by Christopher Green about 5 years ago

  • Status changed from Assigned to Resolved
  • % Done changed from 40 to 100
  • Experiment Mu2e added
  • Experiment deleted (CDF)

Implemented with fhicl-cpp:1a89727 and fhicl-cpp:208dbff.

#15 Updated by Christopher Green about 5 years ago

  • Status changed from Resolved to Closed


Also available in: Atom PDF