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

Should a PortnameSpace be automatically not required if all of its ports are not required either #174

Open
espenfl opened this issue Sep 25, 2018 · 6 comments

Comments

@espenfl
Copy link
Contributor

espenfl commented Sep 25, 2018

If one defines spec.input('somename.someothername', required=False) in a WorkChain and one do not supply that input, it throws and error that it is required. Replacing it to spec.input('somename_someothername', required=False) fixes the problem. Is there a reason for not supporting dots in the name? If so, I guess we should throw another error (if that is possible).

@sphuber
Copy link
Collaborator

sphuber commented Dec 9, 2018

The dots are supported, it is just working slightly differently than you anticipated. The dot is interpreted as a namespace separator, so the line spec.input('somename.someothername', required=False) will be interpreted as "create a namespace somename and within it, create an input port someothername that is not required". However since the default of ports is for them to be required, and so that includes port namespaces, the namespace somename is required. You can see this when looking at the generated ports by calling print(WorkChain.spec().inputs) which should return something like:

"somename": {
        "_attrs": {
            "default": [], 
            "dynamic": false, 
            "help": null, 
            "required": "True", 
            "valid_type": "None"
        }, 
        "someothername": {
            "name": "someothername", 
            "non_db": "False", 
            "required": "False"
        }
    },
}

If you would have passed the inputs {'somename': {'someothername': input}} it should have worked. If you have ideas how to improve the error message or the documentation, let me know

@espenfl
Copy link
Contributor Author

espenfl commented Dec 11, 2018

Great. This is in fact a useful functionality and what I was originally expected from a dot (except I missed the detail about the namespace requirement). Just ejecting an error message that says that the user need to define namespace if using such syntax would help a lot I think. It has been a while, but I seem to remember that no error or warning was ejected. Do you think that would be possible? Not sure where to put it, otherwise I am happy to submit a PR.

@sphuber
Copy link
Collaborator

sphuber commented Dec 11, 2018

I think it is very useful to be able to implicitly create namespaces through the use of dots, instead of being forced to create them explicitly. However, it then becomes difficult to warn the user, that any keywords that are passed, such as required will only apply to the leaf port. We could think if we want these keywords to be applied on the intermediate namespaces as well, but it is not trivial to define when this should be done. For example, if the namespace already exist, should it override settings? Or only when it newly creates it. Either way, I think you will always end up with something that is not a 100% intuitive and will always just need to be documented. Perhaps that is also the best solution here

@espenfl
Copy link
Contributor Author

espenfl commented Dec 11, 2018

Agreed. In fact, the only problem with this is the required=False. It should be possible for the user not to supply the variable, regardless of the namespace being there or not (we are creating it dynamically by definition). Would this behavior be possible to fix? E.g. when a namespace is not there, do not raise an error? I think this would be the cleanest. Then we document the dot feature.

@sphuber
Copy link
Collaborator

sphuber commented Dec 11, 2018

The problem is that currently the namespace itself also has a required attribute. It can for example be the case that a namespace contains multiple ports, some of which are required and some that aren't. Should the requiredness of the namespace be only dependent on the ports it contains? i.e. if it only contains ports that are not required then itself is not required either and as soon it contains one port that is required, then the namespace itself is also required. However, in this scenario, the required attribute on the namespace has no meaning, since it is completely defined by its child ports.
Maybe we should simply get rid of the required attribute for PortNamespaces

@espenfl
Copy link
Contributor Author

espenfl commented Dec 11, 2018

I support this view, that we just inherit. The namespace is probably useless without ports and should also follow their property. If just one port is required, we demand that the namespace is as well.

@sphuber sphuber transferred this issue from aiidateam/aiida-core Oct 4, 2020
@sphuber sphuber changed the title False input port required for workchains Should a PortnameSpace be automatically not required if all of its ports are not required either Oct 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants