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

Feat: pop None inputs specified in overrides #1022

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bastonero
Copy link
Collaborator

Fixes #653

Add the possibility of popping input namespaces by specifying None in the override for the specific namespace. A decorator is added that generalize the concept to any implementation of get_builder_from_protocol.

@bastonero bastonero requested review from mbercx and sphuber March 28, 2024 22:32
@bastonero
Copy link
Collaborator Author

Hi @mbercx @sphuber , it is still somehow draft mode, but I think the basics are there. It works nicely for the test I have written. The limitation of course is that it cannot pop keys within Dict inputs (but I guess that's fine?). I'd love to have your inputs and thoughts.

Fun note: the decorator comes actually from a question to ChatGPT+Copilot, as I just bought it and I was hyper-curious to see the capabilities. Impressive to see what one can achieve with a couple of questions!

@bastonero bastonero force-pushed the feat/pop-overrides branch from cc5ea02 to 75ab490 Compare March 29, 2024 19:20
Fixes #653

Add the possibility of popping input namespaces by specifying
None in the override for the specific namespace. A decorator
is added that generalize the concept to any implementation of
get_builder_from_protocol.
@bastonero bastonero force-pushed the feat/pop-overrides branch from 75ab490 to f6623b0 Compare March 29, 2024 19:49
@mbercx
Copy link
Member

mbercx commented Apr 23, 2024

Thanks @bastonero! But I'm afraid at first glance I'm not too happy with the solution provided by ChatGPT. ^^ Having to wrap every get_builder_from_protocol() method seems tedious, it should just be built into the ProtocolMixin.

Rough outline for what I was thinking (untested or well thought through):

Ideally, we'd first extend the protocol concept to our CalcJobs. No reason you shouldn't be able to define a protocol for a CalcJob. Then, the CalcJob is effectively the "end of the line", i.e. it can't wrap other processes. So at that point you can look for None values and remove them when merging the protocol with the overrides.

The limitation of course is that it cannot pop keys within Dict inputs (but I guess that's fine?).

This limitation is not so nice, I think. I should be able to "unset" an input as well. Even though you can probably always just set the input you want instead, sometimes the final inputs won't make sense to a user. An example would be degauss. Why would this be set if no smearing is used? Now, QE probably doesn't care (though it should), but the input being there might be confusing and just clutters the input file.

EDIT: I'd be happy to work on this, but am currently still too bogged down with other tasks to dedicate any serious time to aiida-quantumespresso. Hopefully I'll be able to wrap up you-know-what soon and really start maintaining the aiida-quantumespresso plugin again.

@bastonero
Copy link
Collaborator Author

Hi @mbercx , thanks for the feedback!

The idea behind this implementation is that you might have a workflow that would perform optional steps. For instance, the PwRelaxWorkChain by default would perform the base_final_scf. In this case, one needs to "pop" it from the builder by hand. This is sub-ideal if we would like to have a new cmdline base on overrides (which I have privately). So for all these cases, one simply needs to put on top the decorator. I don't know if this is possible to do already in the ProtocolMixin, but you get the idea why this is desirable.

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.

Protocols: Consider way of "popping" when overrides is None
2 participants