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

Implement method for JobCalculation to create a restart builder #1962

Merged
merged 2 commits into from
Sep 18, 2018

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Sep 12, 2018

Fixes #230

The get_builder method will return a "clean" builder for the given
JobProcess, without any inputs defined. Here we implement get_builder_restart
for the JobCalculation that will also return a JobProcessBuilder but with
the inputs and options pre-set with those that were used for the original node.
The builder can then immediately be resubmitted to run another instance of that
calculation, while of course giving the chance to change one or more inputs.

For example:

calculation = load_node(<pk>)  # Some completed JobCalculation node
builder = calculation.get_builder_restart()
results = run(builder)

@codecov-io
Copy link

Codecov Report

Merging #1962 into develop will decrease coverage by 0.07%.
The diff coverage is 14.28%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1962      +/-   ##
===========================================
- Coverage    67.56%   67.49%   -0.08%     
===========================================
  Files          324      324              
  Lines        33342    33390      +48     
===========================================
+ Hits         22529    22538       +9     
- Misses       10813    10852      +39
Impacted Files Coverage Δ
aiida/work/process_builder.py 87.69% <0%> (ø) ⬆️
...implementation/general/calculation/job/__init__.py 41.95% <14.58%> (-1.71%) ⬇️
...orm/implementation/general/calculation/__init__.py 80.6% <0%> (+0.86%) ⬆️

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 3893c91...d8addb8. Read the comment docs.

@ltalirz
Copy link
Member

ltalirz commented Sep 12, 2018

Do we need a new method for this or can/should this be handled by passing a parameter of get_builder ?
Also, should we review now or should we wait until it is unblocked?

@sphuber
Copy link
Contributor Author

sphuber commented Sep 12, 2018

I could not implement this with a toggle keyword in get_builder because that one is a class method, and this needs to be an instance method, since you are restarting from a specific JobCalculation instance. Regarding it being blocked: it simply depends on the other PR #1961 , but once that is merged, this implementation should be sufficient, so we can already go through the review process here.

Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

Can we add a test for this new function? (+ the two comments)

builder = self.get_builder()

for port_name, port in self.process().spec().inputs.items():
if port_name == JobProcess.OPTIONS_INPUT_LABEL:
Copy link
Member

Choose a reason for hiding this comment

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

Does this work? This does builder.options = {...}, can this be done directly or should we loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested this manually and it works, because builder.options is just a mapping namespace, so assigning a dictionary works. But I will write some unit tests

setattr(builder, port_name, options)
elif isinstance(port, PortNamespace):
namespace = port_name + '_'
sub = {link.replace(namespace, ''): node for link, node in inputs.items() if link.startswith(namespace)}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure link.replace(namespace, '') is safe, e.g. if the sub-namespace has the same name it would remove both. Better link[len(namespace):] or something equivalent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed

@sphuber sphuber changed the title [BLOCKED] Implement method for JobCalculation to create a restart builder Implement method for JobCalculation to create a restart builder Sep 18, 2018
@coveralls
Copy link

coveralls commented Sep 18, 2018

Pull Request Test Coverage Report for Build 3839

  • 12 of 17 (70.59%) changed or added relevant lines in 1 file are covered.
  • 6 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+7.2%) to 67.878%

Changes Missing Coverage Covered Lines Changed/Added Lines %
aiida/orm/implementation/general/calculation/job/init.py 12 17 70.59%
Files with Coverage Reduction New Missed Lines %
aiida/common/hashing.py 1 91.09%
aiida/orm/implementation/sqlalchemy/group.py 1 88.27%
aiida/orm/implementation/general/calculation/init.py 2 80.6%
aiida/control/postgres.py 2 93.67%
Totals Coverage Status
Change from base Build 3838: 7.2%
Covered Lines: 24124
Relevant Lines: 35540

💛 - Coveralls

The `get_builder` method will return a "clean" builder for the given
`JobProcess`, without any inputs defined. Here we implement `get_builder_restart`
for the `JobCalculation` that will also return a `JobProcessBuilder` but with
the inputs and options pre-set with those that were used for the original node.
The builder can then immediately be resubmitted to run another instance of that
calculation, while of course giving the chance to change one or more inputs.

For example:

    calculation = load_node(<pk>)  # Some completed JobCalculation node
    builder = calculation.get_builder_restart()
    results = run(builder)
@sphuber
Copy link
Contributor Author

sphuber commented Sep 18, 2018

@giovannipizzi I addressed your comments and added some unit tests, so it's ready for a second review

@sphuber sphuber merged commit e41daa3 into aiidateam:develop Sep 18, 2018
@sphuber sphuber deleted the fix_230_job_process_restart branch September 18, 2018 15:52
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.

5 participants