Skip to content

Replace call to deprecated set_ methods with set_option #2361

Merged
sphuber merged 1 commit into
aiidateam:provenance_redesignfrom
sphuber:fix_2360_remove_call_deprecated_set_methods
Jan 6, 2019
Merged

Replace call to deprecated set_ methods with set_option #2361
sphuber merged 1 commit into
aiidateam:provenance_redesignfrom
sphuber:fix_2360_remove_call_deprecated_set_methods

Conversation

@sphuber
Copy link
Copy Markdown
Contributor

@sphuber sphuber commented Dec 19, 2018

Fixes #2360

The Node._set_internal method was still calling deprecated set_
methods for options of the CalcJobNode sub class. These should now
be set through the explicit set_option method. The JobProcess
was likewise violating this rule, when setting the options of the
underlying CalcJobNode it is using as a storage record.

@sphuber sphuber force-pushed the fix_2360_remove_call_deprecated_set_methods branch from 4e20472 to 5ee4864 Compare December 20, 2018 10:35
@sphuber sphuber changed the title [BLOCKED] Replace call to deprecated set_ methods with set_option Replace call to deprecated set_ methods with set_option Dec 20, 2018
@sphuber sphuber force-pushed the fix_2360_remove_call_deprecated_set_methods branch 3 times, most recently from 6486c04 to 232ad54 Compare December 20, 2018 12:42
@coveralls
Copy link
Copy Markdown

coveralls commented Dec 20, 2018

Coverage Status

Coverage increased (+1.2%) to 68.871% when pulling dccc05b on sphuber:fix_2360_remove_call_deprecated_set_methods into 097fa6a on aiidateam:provenance_redesign.

@sphuber sphuber force-pushed the fix_2360_remove_call_deprecated_set_methods branch from 232ad54 to fd975ac Compare December 20, 2018 13:05
giovannipizzi
giovannipizzi previously approved these changes Dec 21, 2018
@giovannipizzi
Copy link
Copy Markdown
Member

Funny, though, that we need to do special treatment for set_computer...

@sphuber
Copy link
Copy Markdown
Contributor Author

sphuber commented Dec 22, 2018

Yes, I wasn't quite sure about this solution, so let's not merge this quite yet and discuss. The problem is if I remember correctly that set_computer is a method of Node and not one of the deprecated explicit option setters defined on the CalcJobNode like all of the other options. I also put in a check for None in the set_option because that was being passed in and tripping up validation, but I'm not sure that is he right thing to do

@sphuber
Copy link
Copy Markdown
Contributor Author

sphuber commented Jan 3, 2019

@giovannipizzi I have looked again in more depth into why we need an exception for computer. The reason is that all options are stored as attributes on the Node except for the computer, which needs to set a foreign key. I looked into whether we could remove computer from the list of available options, but this is not possible I think. For local codes, a user needs to be able to specify what computer to use, and since this is not an actual input node, defined through the use_methods, it has to go in the options. In the old AiiDA, one could call directly call set_computer on the Node, but now that the user no longer directly gets access to the node, setting it through the options is the only way. We could try to rework it some other way, but I think that will end up in code that is uglier than just this single exception in the set_option method.

If you agree as well, I think this is ready to merge

@sphuber sphuber force-pushed the fix_2360_remove_call_deprecated_set_methods branch from b647e39 to cff9db9 Compare January 3, 2019 19:01
The `Node._set_internal` method was still calling deprecated `set_`
methods for options of the `CalcJobNode` sub class. These should now
be set through the explicit `set_option` method. The `JobProcess`
was likewise violating this rule, when setting the options of the
underlying `CalcJobNode` it is using as a storage record.
@sphuber sphuber force-pushed the fix_2360_remove_call_deprecated_set_methods branch from cff9db9 to dccc05b Compare January 4, 2019 14:38
@sphuber sphuber merged commit db31d44 into aiidateam:provenance_redesign Jan 6, 2019
@sphuber sphuber deleted the fix_2360_remove_call_deprecated_set_methods branch January 6, 2019 13:25
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.

3 participants