ORM: replace InputValidationError with ValueError or TypeError

[sphuber]

The InputValidationError was intended for use in CalcJob.prepare_for_submission but is abused in the ORM code to throw when arguments have an invalid value. This should just be communicated through a ValueError.

This will be backwards incompatible and so needs to go in v2.0.0

[ramirezfranciscof]

So, I only currently put a couple of functions in the code, but if I am not misremembering I interpreted that ValueError was for when the actual value of the input was wrong, whereas InputValidationError encompassed other cases, such as wrong types. Are all errors of input verification supposed to raise a ValueError then?

[greschd]

My interpretation is that InputValidationError is for inputs to AiiDA processes, that will have an input link to the process.

The more general ValueError is to be used when it's a wrong "input" (argument) to a Python function. @sphuber happy to be corrected if that's not the intended distinction.

[sphuber]

ValueError and TypeError are Python built-ins and should be used when a function argument is of incorrect value or type, respectively. The aiida.common.exceptions.InputValidationError is an AiiDA exception that was originally intended to be used for input validation errors in CalcJobs. Unfortunately, the InputValidationError does not subclass the ValueError so anyone catching that will miss it. Since the ValueError is perfectly fine, I simply don't think we need a custom exception in our ORM.

[ramirezfranciscof]

Are there any other input error types to consider? Else maybe I would change the the title and/or the text to include "replace InputValidationError with ValueError or TypeError" since at least I may have introduced a couple of those (⌒_⌒;) .

[sphuber]

No typically when validating method arguments, it will be either of incorrect type or value, so that is usually all one needs

[sphuber]

Coming back to this, I would like to address this for the upcoming v2.0 but not sure how we can do this now without breaking backwards compatibility. If there is code out there that is catching InputValidationError from ORM code, then that would break if we switch it to a ValueError or TypeError since it subclasses neither. I am also not even sure how to go about providing a deprecation pathway for this. So in the end I think we should make a simple decision: either we take the risk and simply replace the incorrect exceptions and potentially break existing code. Or we live with this inconsistency forever.

A search across the aiidateam organization for except InputValidationError shows that it is not used in any of the plugins, just in aiida-core. Also a broader search for just InputValidationError doesn't show any code catching it outside of aiida-core. Maybe it would be ok then to make the replacement assuming other plugins will not catch it either.

EDIT: did some similar searching in most well-known plugins and found just one occurrence so far in aiida-fleur.

https://github.com/JuDFTteam/aiida-fleur/blob/a5489810094d201ef672c4cfb4b4f86b14147443/aiida_fleur/tools/common_fleur_wf.py#L49

But not even sure that that could actually be thrown by that code. In any case, that entire function could be replaced by load_code 😄

[sphuber]

This was accepted during the meeting of Thursday April 29 2021

[ltalirz]

Reading through the issue and the PR #4888 now, I am wondering: what prevents this issue from recurring?
To me, the underlying problem is that the term InputValidationError does not make the specialization to processes obvious and might be picked up by developers for different purposes again in the future.

If it really matters to limit its use to the input of processes, I would suggest to include this in the name (e.g. ProcessInputValidationError).

However, since addressing this issue involves backward-incompatible changes, I would also like to understand the use case that gave rise to this issue - @sphuber were you catching ValueErrors and ran into problems because the InputValidationError was not caught?
Or were you catching InputValidationErrors and missing ValueErrors?

[sphuber]

Reading through the issue and the PR #4888 now, I am wondering: what prevents this issue from recurring?

Reviews on pull requests. There is no need to be using this special exception when a simple ValueError or TypeError does the job.

To me, the underlying problem is that the term InputValidationError does not make the specialization to processes obvious and might be picked up by developers for different purposes again in the future.

These exceptions weren't necessarily designed to be used outside of aiida-core. I don't think people should be using these but would definitely not change the name now because that would definitely be breaking existing plugins.

If it really matters to limit its use to the input of processes, I would suggest to include this in the name (e.g. ProcessInputValidationError).

However, since addressing this issue involves backward-incompatible changes, I would also like to understand the use case that gave rise to this issue - @sphuber were you catching ValueErrors and ran into problems because the InputValidationError was not caught?
Or were you catching InputValidationErrors and missing ValueErrors?

There was no immediate bug that led to this, other than the use of these exceptions are not necessary and even if we wanted to use a specific exception, according to the docstring this was clearly an incorrect one. Its usage has slipped into the ORM a long time ago when people weren't aware and maybe there were no pull requests yet. I am talking way before v1.0 even. By having these exceptions in the ORM, now we are forcing users to a) know that these specific ones can be thrown and b) have to import this exception from aiida-core to explicitly catch it, because it doesn't subclass the base exception that one would expect for a method argument with an incorrect value.