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

BaseRestartWorkChain: Fix bug in _wrap_bare_dict_inputs #5757

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Nov 12, 2022

In ae79c89, released with v2.1.0, the InputPort class was updated to automatically add None as a valid type to the valid_type tuple if the port is not required. This unintentionally broke the BaseRestartWorkChain._wrap_bare_dict_inputs method which is supposed to wrap bare dictionaries in the inputs if the corresponding port accepts a Dict node. However, it was checking that the valid_type was equal to Dict instead of checking that the tuple contains the Dict type. As a results, bare dictionaries were no longer automatically wrapped in Dict node after the change mentioned above.

Here the logic is updated to check whether the Dict type is in the valid_type tuple.

@sphuber sphuber requested a review from mbercx November 12, 2022 12:58
@codecov
Copy link

codecov bot commented Nov 12, 2022

Codecov Report

Base: 79.87% // Head: 79.87% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (46dbb86) compared to base (1e77f2a).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@                Coverage Diff                @@
##           release/2.1.2    #5757      +/-   ##
=================================================
+ Coverage          79.87%   79.87%   +0.01%     
=================================================
  Files                496      496              
  Lines              37027    37028       +1     
=================================================
+ Hits               29573    29574       +1     
  Misses              7454     7454              
Impacted Files Coverage Δ
aiida/engine/processes/workchains/restart.py 84.50% <100.00%> (+0.63%) ⬆️
aiida/transports/plugins/local.py 82.59% <0.00%> (-0.24%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sphuber sphuber changed the base branch from main to release/2.1.2 November 14, 2022 11:12
In ae79c89, released with `v2.1.0`, the
`InputPort` class was updated to automatically add `None` as a valid
type to the `valid_type` tuple if the port is not required. This
unintentionally broke the `BaseRestartWorkChain._wrap_bare_dict_inputs`
method which is supposed to wrap bare dictionaries in the inputs if the
corresponding port accepts a `Dict` node. However, it was checking that
the `valid_type` was equal to `Dict` instead of checking that the tuple
contains the `Dict` type. As a results, bare dictionaries were no longer
automatically wrapped in `Dict` node after the change mentioned above.

Here the logic is updated to check whether the `Dict` type is in the
`valid_type` tuple.
@sphuber sphuber force-pushed the fix/base-restart-workchain branch from e1c5bdf to 46dbb86 Compare November 14, 2022 11:13
Copy link
Member

@mbercx mbercx left a comment

Choose a reason for hiding this comment

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

Code looks good, and I tested it in the field and all seems to work fine.

In principle the change that caused this bug is not fully backwards-compatible, but perhaps it's ok. Not sure how many other plugins use code that relies on checking the valid_type of an input port that is not required. 🤷

@sphuber
Copy link
Contributor Author

sphuber commented Nov 14, 2022

Not sure how many other plugins use code that relies on checking the valid_type of an input port that is not required.

Since the change is quite useful, I am tempted to just keep it and fix the bug in the BaseRestartWorkChain. If we get more reports from other plugins also breaking, we can always still revert it.

@sphuber sphuber merged commit 11304eb into release/2.1.2 Nov 14, 2022
@sphuber sphuber deleted the fix/base-restart-workchain branch November 14, 2022 15:33
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