Code example for alternative ways of passing setting of a relaxation workchain

[zhubonan]

This is a code example, not meant to be merged as it is.

Description

This is for continuing the discussion in #510. Perhaps a separate issue should be raised.

This PR serves as an example of an alternative way of passing the settings inputs for VaspRelaxWorkChain. Which many other work chains would face the same problem. If there are many settings/flags to be passed for controlling the workflows, it is better to store them in a single Dict node, rather than individual Node inside a large PortNamespace. The latter would create a lot more input nodes for the work chain, which is not space-efficient and would make submitting the work chain very slow.

There are some other functionalities that I implemented in this code example, such as bootstrapping a hybrid calculation, modify parameters for the final static run, and additional checks. But the main point is to discuss the design of the input interface.

[codecov]

Codecov Report

Merging #516 (8660629) into develop (3355ea1) will decrease coverage by 5.67%.
The diff coverage is 0.00%.

❗ Current head 8660629 differs from pull request most recent head c8c25d5. Consider uploading reports for the commit c8c25d5 to get more accurate results

@@             Coverage Diff             @@
##           develop     #516      +/-   ##
===========================================
- Coverage    72.74%   67.07%   -5.66%     
===========================================
  Files           57       58       +1     
  Lines         6612     7171     +559     
===========================================
  Hits          4809     4809              
- Misses        1803     2362     +559     

Impacted Files	Coverage Δ
aiida_vasp/workchains/alt_relax.py	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3355ea1...c8c25d5. Read the comment docs.

[atztogo]

@zhubonan, I feel the implementation of typed_field is tricky. Is this a popular approach?

[zhubonan]

@atztogo I guess you mean the way of using properties for attributes? I think that is a valid pythonic approach and ProcessBuilder uses it, although the getter and setter methods are attached from the PortNamespace supplied. That is because the ProcessBuilder instance needs to be instantiated dynamically based on the spec of the Process. Here, the OptionHolder subclasses are static so we can just define it inside the class definition.

It is probably better to split typed_field into bool_field, float_field, int_field etc. though.

[atztogo]

@zhubonan, thanks for your answer. My question may be even more trivial. I see it works from https://docs.python.org/3/library/functions.html#property, but it was difficult for me to see what self in typed_field is intuitively. So my question was if there exists another popular way to implement this feature, and if this is a widely known approach or not.

[zhubonan]

I am not aware of another way of achieving the same functionality, at least without making it more complicated. The key feature is that when one can do:

obj.bool_field = 3.0

The setter method of the property bool_field would be called with 3.0 and act upon it, here it would raise an exception as 3.0 is not a bool value. This additional step is what makes it different from simplifying using an attribute bool_field. In theory, one could also override __setattribute__, but it would make the logic too complicated.

The self argument in getter is the object instance whose the property needs to be returned, just like in https://docs.python.org/3/library/functions.html#property. For example, c.getx() is the same as C.getx(c), and c.x is the same as C.getx(c).

[atztogo]

I see your point. After some reading about python descriptor (https://docs.python.org/3/howto/descriptor.html#properties), I understood how property works (at a certain level), and some more reading, I understood "property factory" is a known pattern. So I agree your code is well designed. Due to lack of my knowledge, I thought it looked tricky.

During the reading, I found, from python3.6, __set_name__ is introduced. https://realpython.com/python-descriptors/ By this, you can avoid passing attribute name as a string as found around here (steps, etc),

aiida-vasp/aiida_vasp/workchains/alt_relax.py

Line 301 in 8660629

steps = typed_field('steps', (int,), 'Number of relaxation steps to perform (eg. NSW)', 60)

An example of the usage is found at the last section of https://realpython.com/python-descriptors/. I feel the descriptor class approach is more intuitive when reading the code. Some discussion about comparison between the property factory and the descriptor class approach is found in the book Fluent Python, too. Considering also the flexibility and extendability, it may be good to consider using the descriptor class approach, which may be encouraged by the fact that minimum python version dependency of aiida-vasp is now python3.6.

[zhubonan]

@atztogo I see. Yeah using the Descriptor would be a better approach. I need to refactor the code anyway so might as well adapt it. In the end, the user interface should be the same.

[zhubonan]

@atztogo I have updated the code using descriptors. Now the syntax is better with more code reuse. One thing to keep in mind is that we still have to inherit Option from property, this is because IPython checks if a class attribute is a property when doing the introspect and handles it differently to display the correct information (of the property, rather than the returned object).

Please try it out. Some examples:

from aiida_user_addons.vworkflows.relax import RelaxOptions
opts = RelaxOptions()
opts.  # Tab to see options
opts.algo?  # IPython introspect should work
opts.algo = 'fast'  # Should raise an error only, 'cg' or 'rd' are allowed
opts['fake_option'] = 'foo'  # Not allowed
opts.fake_option = 'foo'  # No error for now - I don't want to overload __setattr___...
opts.to_dict()  # Detect unsupport option `fake_option` been set, and raises an exception.

print(RelaxOptions.get_description())  # Prints help strings for all options

[atztogo]

Super!

One thing to keep in mind is that we still have to inherit Option from property, ...

I guess this was information difficult to find...