Instantiation of SinglefileData class does not fail if file is set to None

[astamminger]

Hi all,

I just fiddled around with the SinglefileData class. While doing so I ended up making a stupid mistake which effectively resulted in the class being instantiated as SinglefileData(None) without me noticing it. Even though instantiating the class gave me no errors, calling the get_content() or open() methods resulted in some dubious and not very helpful error message, i.e.

AttributeError: filename

not remotely related to the fact that the class was instantiated with no file at all.

I have two questions:

• Is this implementation done like that on purpose?
• is there a use-case for doing that?

If that was not done on purpose / there's no use-case: It would have saved me a lot of debugging time if i would not have been able to instantiate the class like that in the first place. Maybe we can improve the error message giving more information why this has happened or even prevent anyone from instantiating the class with file=None.

Steps to reproduce

If you want to reproduce, here's the snippet i used:

from aiida.orm import SinglefileData
sfd = SinglefileData(None)
with sfd.open(mode='r') as fh:
    content = fh.read()

Environment

• Operating system [e.g. Linux]: Linux
• Python version [e.g. 3.7.1]: 3.8.13
• aiida-core version [e.g. 1.2.1]: 2.0.3

[sphuber]

This design stems from the very beginning of AiiDA (before I was a part of the project, so I can be wrong about the reasoning). As you can see in the current code, the Node.store method calls _validate. I think the original authors preferred, or simply defaulted, to this design of making sure the node content was consistent just before storing. Maybe they saw value in being able to instantiate a node "empty" and just validate once. This is the case not just for SinglefileData, but many other classes, such as KpointsData and TrajectoryData. @giovannipizzi do you remember any specific design considerations here?

I personally disagree with this approach and think a node should only be instantiable in a valid state. We would need to keep the check before storing obviously as the instance can be modified after construction but before storing. The example you give is one of the main arguments for this stance.

So I would be in favor of changing the behavior, but as always it is the backwards-compatibility that would be the main stumbling block here. If we were to stop accepting SinglefileData() being constructed without a file, I am pretty sure that that would break some code out there. Not sure how common this is though.

[astamminger]

Thanks for the quick reply @sphuber

Yes, I already had the feeling that this could have been done on purpose, but let's see what @giovannipizzi has to say. If the behavior cannot be changed, due to the reasons you mentioned, IMHO it would also be sufficient to just make the raised error a bit more specific inside the affected methods.

Once it is decided what to do, I can take care of this and prepare a PR.

//Edit: Or handle it similar to the TrajectoryData and simply document it in the docstring of the methods that it will raise an AttributeError if no file was set.