jsonargparse icon indicating copy to clipboard operation
jsonargparse copied to clipboard

Option to fail on failure to initialize classes

Open JoostvDoorn opened this issue 1 year ago • 3 comments

🚀 Feature request

Add an option to fail on class initialization.

Motivation

Currently if a config contains a class_path with init_args, and it fails to initialize the class it will ignore the error, and just return a dict/namespace. If the class contains a error it's unclear what the issue is requiring the user to use a debugger or to test it outside of the script that uses jsonargparse.

See the code (on line 1054-1055 any exception is ignored): https://github.com/omni-us/jsonargparse/blob/7da8b8247c3e447386da599e441d4ac5cfee67e9/jsonargparse/typehints.py#L1044-L1055

Note: I am not aware of the full impact, and I think it's possible that simply raising this exception always is not enough as it might fail too often as well in valid use cases. E.g. check_values in https://github.com/omni-us/jsonargparse/blob/927ec027f41520cc768d501387ea678a096848c8/jsonargparse/core.py#L1074 raises an exception for a case which I think is valid.

Pitch

I would like to have an option to fail in cases where initializing a class fails, and raise the exception of the underlying code.

Alternatives

An alternative is to fail always, but this could be considered a breaking change.

JoostvDoorn avatar Apr 19 '23 09:04 JoostvDoorn

@JoostvDoorn thank you for reporting!

From an initial thought, I don't like the idea of having an option to fail or not. The code that you reference is for when there is no type hint or it is Any. The idea to not fail is because a dict is also valid according to Any. But I agree it is not good that to figure out a problem the only way is debugging. An alternative to inform the CLI user could be a warning.

Just out of curiosity, Is there a reason why you can't have a proper class type hint for it?

Even with a proper class type hint, I believe I had not considered in detail what happens when instantiation fails. Possibly there are other cases apart from Any in which the behavior should be improved.

mauvilsa avatar Apr 21 '23 05:04 mauvilsa

Just out of curiosity, Is there a reason why you can't have a proper class type hint for it?

In some cases type hints are missing when we need to initialize a class later (so not by jsonargparse), and we pass the arguments as a dict[str, Any]. Most common this is with datasets in a datamodule where we pass the dataset arguments in this way. Still we want the classes in the arguments to be initialized as classes.

I think it should be possible to treat the "class_path": variable as a reserved argument and error on any case where it fails to initialize the class.

JoostvDoorn avatar Aug 08 '23 08:08 JoostvDoorn

@mauvilsa I am facing the same issue.

For me the application is for instance, I define the class

class Module:

  def __init__(self, module: Any, checkpoint: str):
      self.module = module
      self.checkpoint = checkpoint

Now I use a list modules: List[ModuleConfig], to handle model loading, e.g. load the model, then its corresponding checkpoints,... As I want to have a generic model loader, I am using Any here.

rob-hen avatar Apr 30 '24 08:04 rob-hen