Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in aiida.engine.utils.instantiate_process #5952

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Mar 31, 2023

If the process argument receives an unsupported type that is not a class, the final check issubclass(process, Process) would raise:

TypeError: issubclass() arg 1 must be a class

which is not very useful to the user. This is fixed by first checking that the argument is a class to begin with. At least now when an invalid type is provided, the exception contains the invalid type:

invalid process <class \'bool\'>, needs to be Process or ProcessBuilder'

If the `process` argument receives an unsupported type that is not a
class, the final check `issubclass(process, Process)` would raise:

    TypeError: issubclass() arg 1 must be a class

which is not very useful to the user. This is fixed by first checking
that the argument is a class to begin with. At least now when an invalid
type is provided, the exception contains the invalid type:

    invalid process <class \'bool\'>, needs to be Process or ProcessBuilder'
@sphuber sphuber requested a review from unkcpz March 31, 2023 15:32
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sphuber Thanks! The change looks good.
But I am curious where you encounter this, I can not think of a case to pass the bool type as process to this function. The input argument type hint also does not reveal this as a case. It makes more sense if the type check is at the beginning of the function to make sure the process pass in conforms with the type hint.

@sphuber
Copy link
Contributor Author

sphuber commented Apr 2, 2023

@sphuber Thanks! The change looks good. But I am curious where you encounter this, I can not think of a case to pass the bool type as process to this function. The input argument type hint also does not reveal this as a case. It makes more sense if the type check is at the beginning of the function to make sure the process pass in conforms with the type hint.

The boolean is just for the test case. I don't remember exactly, but I think in the case where this was encountered a class instance of something besides a Process or ProcessBuilder was passed, or even a method. For example, I think the following would have hit the exception:

builder = load_code('pw@localhost').get_builder
submit(builder)

Note how the () call at the end of get_builder was forgotten, and so now the bound method is passed and issubclass raises, since it is not a class.

It was something like that, but seemed like an honest mistake that could happen and the current resulting error message was not helpful whatsoever.

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair, thanks for the explanation!

@sphuber sphuber merged commit 01dbb23 into aiidateam:main Apr 3, 2023
@sphuber sphuber deleted the fix/instantiate-process branch April 3, 2023 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants